Conversation
📝 WalkthroughWalkthroughThis PR performs a comprehensive refactoring upgrading the Java/Kotlin build to version 21, introduces a new motion-store module with database persistence for projects, refactors configuration management from static mutable to immutable provider-based approach, restructures album art and lyrics APIs to use repository pattern, makes worker lifecycle methods suspend-friendly, and significantly expands UI with project management and interactive lyric selection. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Mobile App
participant LyricsVM as LyricsViewModel
participant Repo as LyricsRepository
participant Service as LrcLibApiService
participant HTTP as OkHttp Client
participant API as LrcLib API
Client->>LyricsVM: searchLyrics(trackName, artistName)
LyricsVM->>LyricsVM: uiState = Loading
LyricsVM->>Repo: searchLyrics(SearchParams)
Repo->>Service: search(SearchParams)
Service->>HTTP: GET /search?track_name=X&artist_name=Y
HTTP->>API: HTTP Request
API-->>HTTP: JSON Response
HTTP-->>Service: Response Body
Service->>Service: Parse JSON via Gson
Service-->>Repo: Result.success(List<LyricsResponse>)
Repo-->>LyricsVM: Result.success(lyrics)
LyricsVM->>LyricsVM: uiState = Success(lyrics)
LyricsVM-->>Client: StateFlow emits updated state
Client->>Client: Render lyrics list
sequenceDiagram
participant App as LyricsApp
participant DBMgr as DatabaseManager
participant DB as SQLite
participant DAO as MotionProjectDao
participant Store as ProjectsViewModel
App->>DBMgr: init(context, schemas=[MotionProject])
DBMgr->>DB: onCreate(db)
DB->>DB: Execute CREATE TABLE MotionProject
Store->>DAO: loadProjects()
DAO->>DB: query(tableName, selection, args)
DB-->>DAO: Cursor<MotionProject>
DAO->>DAO: fromCursor() for each row
DAO-->>Store: List<MotionProject>
Store->>Store: _projects.emit(list)
Store-->>UI: StateFlow<List<MotionProject>>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @tejpratap46, 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 delivers a substantial overhaul of the project's foundational components and a targeted refactoring of the 'lyrics-maker' module. The core build system and language environment have been modernized with upgrades to Gradle, Android Gradle Plugin, Kotlin, and JDK 21, setting the stage for future development. A new 'motion-store' module introduces local data persistence capabilities, while the MotionConfig system has been made more flexible. The 'lyrics-maker' module benefits from a complete rewrite of its network layer, adopting a more maintainable and scalable architecture. These changes collectively improve the project's stability, performance, and development experience. 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
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Critical Issues Found
This PR introduces SDUI support for video storage but contains 8 blocking defects that must be fixed before merge:
Security (1)
- Hardcoded database credentials in DatabaseProvider expose no-op authentication
Crash Risks (3)
- Resource leaks in LrcLibApiServiceImpl will cause OutOfMemoryError
- DatabaseProvider constructor prevents instantiation, making module unusable
- Invalid Gradle syntax in motion-store build.gradle will fail compilation
Logic Errors (2)
- MotionConfig race condition breaks multi-instance configurations
- Connection pool misconfiguration destroys HTTP performance benefits
Summary
The refactoring from data object to data class for MotionConfig and the migration from Ktor to OkHttp are architecturally sound, but implementation issues block merge. Focus on fixing the resource leaks and database provider instantiation first as these cause runtime crashes.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionConfig.kt
Show resolved
Hide resolved
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/di/OkHttpProvider.kt
Outdated
Show resolved
Hide resolved
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/di/OkHttpProvider.kt
Show resolved
Hide resolved
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/DatabaseProvider.kt
Outdated
Show resolved
Hide resolved
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/DatabaseProvider.kt
Outdated
Show resolved
Hide resolved
.../src/main/java/com/tejpratapsingh/lyricsmaker/data/api/lrclib/client/LrcLibApiServiceImpl.kt
Outdated
Show resolved
Hide resolved
.../src/main/java/com/tejpratapsingh/lyricsmaker/data/api/lrclib/client/LrcLibApiServiceImpl.kt
Outdated
Show resolved
Hide resolved
tejpratap46
force-pushed
the
feat/implement-sdui
branch
from
d0f458e to
3a9034b
Compare
![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 significant refactoring and new features. Key changes include upgrading Gradle and Kotlin versions, refactoring MotionConfig to be instance-based rather than a singleton object, and replacing Ktor with OkHttp/Gson in the lyrics-maker module along with a repository pattern. A new motion-store module for database persistence using Nitrite and an sdui module for server-driven UI have been added. The changes are generally positive, improving code structure and updating dependencies. My review focuses on potential issues like thread blocking, incomplete implementations in new modules, and making the code more robust and maintainable.
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/DatabaseProvider.kt
Outdated
Show resolved
Hide resolved
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/MotionProject.kt
Outdated
Show resolved
Hide resolved
modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionConfig.kt
Outdated
Show resolved
Hide resolved
...s-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt
Outdated
Show resolved
Hide resolved
...maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt
Outdated
Show resolved
Hide resolved
modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/VideoAspectRatioParser.kt
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (15)
modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/BaseContourMotionView.kt (1)
39-45:⚠️ Potential issue | 🟡 MinorPre-existing off-by-one:
0..this.childCountiterates one index too far.Not introduced by this PR, but
0..childCountis an inclusive range in Kotlin, sogetChildAt(childCount)returnsnull. Theis MotionViewcheck prevents a crash, but it's semantically incorrect. The idiomatic fix is0 until childCount.Suggested fix
- for (i in 0..this.childCount) { + for (i in 0 until this.childCount) {modules/pytorch-motion-ext/build.gradle (1)
1-4:⚠️ Potential issue | 🔴 CriticalDo not remove the
kotlin-androidplugin; this module contains Kotlin source files.The module has 9 Kotlin source files (including
PyTorchImageProcessor.kt,ModelTypes.kt,RemoveBg.kt, and others), but the build.gradle removes thekotlin-androidplugin. This will cause compilation failures. Either retain the Kotlin plugin or convert all source files to Java.modules/jcodec-motion-ext/build.gradle (1)
1-4:⚠️ Potential issue | 🔴 CriticalRestore the Kotlin Android plugin—this module has .kt source files that will fail to compile without it.
The
kotlin.androidplugin was removed but the module contains three Kotlin source files:
src/main/java/com/tejpratapsingh/motionlib/jcodec/FileExtension.ktsrc/main/java/com/tejpratapsingh/motionlib/jcodec/JCodecVideoProducerAdapter.ktsrc/test/java/com/tejpratapsingh/motionlib/jcodec/ExampleUnitTest.ktWithout the Kotlin plugin, these files cannot be compiled.
modules/sdui/build.gradle (1)
1-4:⚠️ Potential issue | 🔴 CriticalAdd the Kotlin Android plugin back to the build.gradle file.
Multiple Kotlin source files exist in the module (10 .kt files including SduiRenderer.kt, ViewFactory.kt, ConfigParser.kt, ContainerFactory.kt, ImageFactory.kt, TextFactory.kt, ImageLoader.kt, VideoAspectRatioParser.kt, and test files), but the Kotlin Android plugin has been removed. These files will not compile without it.
modules/tensorflow-motion-ext/build.gradle (1)
1-4:⚠️ Potential issue | 🔴 CriticalAdd the Kotlin plugin back to build.gradle—the module contains 7 Kotlin source files that cannot compile without it.
The module has 6 Kotlin sources in
src/main/java/and 1 test source insrc/test/java/. Removing the Kotlin Android plugin will break the build. Either restore the Kotlin plugin or migrate all Kotlin files to Java.modules/3d-opengl-renderer/build.gradle (1)
1-4:⚠️ Potential issue | 🔴 CriticalRemove
kotlin-androidplugin must be restored or all Kotlin sources removed.This module contains 5 Kotlin source files:
MotionOpenGlView.ktObjModel.ktObject3DToBitmapRenderer.ktOffscreenRenderer.ktExampleUnitTest.ktRemoving the
kotlin-androidplugin will cause compilation to fail. Either restore the plugin or migrate these files to Java.modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/MotionComposerView.kt (1)
35-36:⚠️ Potential issue | 🟡 MinorPre-existing off-by-one:
0..this.childCountshould be0 until this.childCount.The
..range is inclusive, sogetChildAt(childCount)returnsnull, which won't crash here (theis MotionViewcheck handles it), but iterating one extra index is unnecessary.Proposed fix
- for (i in 0..this.childCount) { + for (i in 0 until this.childCount) {modules/ffmpeg-motion-ext/build.gradle (1)
1-4:⚠️ Potential issue | 🔴 CriticalRestore the
kotlin-androidplugin—this module contains Kotlin source files.The build will fail to compile the Kotlin sources in this module:
src/main/java/com/tejpratapsingh/motionlib/ffmpeg/FfmpegVideoProducerAdapter.ktsrc/main/java/com/tejpratapsingh/motionlib/ffmpeg/utils/FFMpegExtensions.ktsrc/main/java/com/tejpratapsingh/motionlib/ffmpeg/video/FFMpegVideoFrameView.ktsrc/test/java/com/tejpratapsingh/motionlib/ffmpeg/ExampleUnitTest.ktAdd
kotlin-androidback to the plugins block.modules/3d-filament-renderer/build.gradle (1)
1-4:⚠️ Potential issue | 🔴 CriticalRe-add the
kotlin-androidplugin tobuild.gradle.This module contains Kotlin source files (
Filament3dView.kt,FilamentOffscreenCapturer.kt,ExampleUnitTest.kt). Removing thealias(libs.plugins.kotlin.android)plugin will prevent these files from compiling. The plugin must be re-added.modules/motionlib/build.gradle (1)
1-4:⚠️ Potential issue | 🔴 CriticalRemove the
kotlin.androidplugin from the published build configuration at your own risk.This module contains 33 Kotlin source files. Removing the
kotlin.androidplugin will break compilation of all.ktfiles in the module. Restorealias(libs.plugins.kotlin.android)to the plugins block unless the entire codebase is being migrated to Java.modules/templates/build.gradle (1)
1-4:⚠️ Potential issue | 🔴 CriticalThe
kotlin-androidplugin should not be removed: the module contains Kotlin sources.
modules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/ExampleUnitTest.ktexists in this module. Removing thekotlin-androidplugin while Kotlin test files are present will prevent proper Kotlin compilation. Either restore the plugin or ensure Kotlin compilation is handled through an alternative mechanism.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SyncedLyricsSelector.kt (1)
93-93:⚠️ Potential issue | 🟠 MajorSide-effect in composition:
onSelectionChangedcalled during recomposition.
onSelectionChanged(selected)on line 93 is invoked directly inside the composable body, meaning it fires on every recomposition whenselection != null. Side-effects in Compose should use effect handlers likeLaunchedEffectto avoid repeated/unintended invocations.Proposed fix
- if (selection != null) { - val selected = viewModel.lyrics.subList(selection!!.minIndex, selection!!.maxIndex + 1) + val selected = selection?.let { + viewModel.lyrics.subList(it.minIndex, it.maxIndex + 1) + } + if (selected != null) { Surface(tonalElevation = 2.dp) { ... } HorizontalDivider() - onSelectionChanged(selected) } + + LaunchedEffect(selection) { + if (selected != null) { + onSelectionChanged(selected) + } + }modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt (1)
79-100:⚠️ Potential issue | 🟠 Major
runBlockinginside a View'sinitblocks the main thread and risks ANR.Network calls (album art URL fetch + bitmap download) are performed synchronously on the main thread via
runBlocking. This can freeze the UI for several seconds or trigger an Application Not Responding dialog. Consider launching a coroutine on a background scope and posting the bitmap back to the view.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SearchLyricsCompose.kt (1)
91-180:⚠️ Potential issue | 🟠 MajorError state is silently swallowed — users get no feedback on search failure.
The
whenblock only matchesSuccessexplicitly;LyricsUiState.Errorfalls through to theelsebranch, which just shows recent searches. When the API call fails, the loading spinner vanishes and the user sees no indication of what went wrong.Consider handling
Errorexplicitly:Proposed fix
when (val state = uiState.value) { is LyricsUiState.Success -> { // ... existing success rendering ... } - else -> { + is LyricsUiState.Error -> { + Text( + text = state.message, + color = MaterialTheme.colorScheme.error, + style = MaterialTheme.typography.bodyMedium, + modifier = Modifier.padding(vertical = 8.dp), + ) + // fall through to show recent searches below + } + + else -> {} + } + + if (uiState.value !is LyricsUiState.Success) { Spacer(modifier = Modifier.height(16.dp)) Text("Recent Searches:", style = MaterialTheme.typography.titleMedium) - LazyColumn { + LazyColumn(modifier = Modifier.fillMaxSize()) { items(recentSearches.value.size) { idx -> // ... existing recent searches rendering ... } } - } }modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt (1)
103-113:⚠️ Potential issue | 🔴 Critical
selectedLyricsgetter crashes withNoSuchElementExceptionwhen the list is empty.
fielddefaults toemptyList(), and line 105 callsfield.first()unconditionally. Any access toselectedLyricsbefore it is explicitly set (or if set to an empty list) will crash at runtime.Proposed fix
var selectedLyrics: List<SyncedLyricFrame> = emptyList() get() { + if (field.isEmpty()) return emptyList() val firstFrame = field.first().frame return field .map {
🤖 Fix all issues with AI agents
In @.idea/compiler.xml:
- Line 4: The IDE bytecodeTargetLevel is set to "21" but module
compileOptions/sourceCompatibility and targetCompatibility are still
JavaVersion.VERSION_11; update one side so they match. Either change the
<bytecodeTargetLevel target="21"> entry to target="11" to match existing
compileOptions/sourceCompatibility and targetCompatibility, or update all module
build.gradle settings (sourceCompatibility/targetCompatibility and any
compileOptions.release) to Java 21 so Gradle and the IDE are consistent; adjust
the corresponding symbol(s) bytecodeTargetLevel and the Gradle properties
sourceCompatibility/targetCompatibility/compileOptions.release accordingly.
In
`@modules/3d-filament-renderer/src/main/java/com/tejpratapsingh/motionlib/filamentrenderer/Filament3dView.kt`:
- Line 116: The aspect ratio calculation uses integer division because
provideMotionConfig().aspectRatio.width and .height are Ints, causing a zero or
wrong ratio; change the expression in Filament3dView (where you call
setProjection) to perform floating-point division by converting width and/or
height to Double before the division (e.g., cast the numerator or both sides to
Double or multiply by 1.0) so the resulting aspect ratio is a proper Double.
In
`@modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionConfig.kt`:
- Around line 3-11: The MotionConfig data class currently mutates global state
in its init block and uses mutable properties; remove the init block that writes
MotionConfigStore.motionConfig, change the properties in MotionConfig
(aspectRatio, fps, outputQuality) from var to val to make the config immutable,
and add/use an explicit registration API on MotionConfigStore (e.g.,
register(config: MotionConfig) or setMotionConfig(config: MotionConfig)) so
callers must explicitly call MotionConfigStore.register(...) after constructing
or copying a config; update any call sites that relied on implicit registration
to call the new register method.
- Around line 13-17: MotionConfigStore's mutable field motionConfig is not
thread-safe and must be made safe: replace the plain var access with a
thread-safe mechanism (e.g., change motionConfig to an
AtomicReference<MotionConfig?> or mark it `@Volatile` and synchronize
writes/reads) so provideMotionConfig() reads a safe snapshot; update
MotionConfigStore and provideMotionConfig() accordingly, ensuring any
setters/update code (if present) use AtomicReference.set/get or synchronized
blocks to avoid data races.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/albumart/client/AlbumArtRemoteDataSourceImpl.kt`:
- Around line 29-32: The Lucene query in AlbumArtRemoteDataSourceImpl is
interpolating unescaped trackName and artistName into the query string (the
addQueryParameter call building "recording:\"$trackName\" AND
artist:\"$artistName\""), which can break or corrupt searches; fix by
introducing and using a helper (e.g., escapeLuceneSpecialChars or
escapeForLuceneQuery) to escape Lucene special characters (\, ", + - && || ! ( )
{ } [ ] ^ ~ * ? : etc.) in trackName and artistName before interpolation, and
replace the raw variables with the escaped values when calling addQueryParameter
so the query is always well-formed.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/lrclib/client/LrcLibApiServiceImpl.kt`:
- Around line 50-60: The response is not closed on error paths in the HTTP call
handling inside the search and get methods of LrcLibApiServiceImpl (the block
that calls client.newCall(request).execute()); wrap the OkHttp Response in
response.use { ... } (or explicitly call response.close() on all return/error
paths) so the body is always closed even when response.isSuccessful is false,
and move the success/failure checks and body.string() extraction into that use
block to return Result accordingly.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/LyricsActivity.kt`:
- Line 79: The duration string uses integer division causing truncated seconds;
change the expression in LyricsActivity.kt that builds the "Duration" line
(currently using (end - start) / provideMotionConfig().fps) to perform
floating-point division by converting operands to Double (e.g., (end -
start).toDouble() / provideMotionConfig().fps.toDouble()) and format the result
(for example to 2 decimal places) so the displayed seconds include fractional
precision; reference variables end, start and the provideMotionConfig() call
when making the change.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt`:
- Around line 65-90: Change the bare lifecycleScope.launch collection in
SearchActivity to a lifecycle-aware collector: inside the activity lifecycle
(e.g., onCreate/onStart) use lifecycleScope.launch {
repeatOnLifecycle(Lifecycle.State.STARTED) { collect lyricsViewModel.uiState } }
so lyricsViewModel.uiState is only collected while the activity is visible;
update the collector that handles LyricsUiState.Error, .Loading, .Success,
.Initial to run inside that repeatOnLifecycle block and add the necessary import
for repeatOnLifecycle.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SyncedLyricsSelector.kt`:
- Line 151: The code uses integer division and repeatedly calls
provideMotionConfig() per item; hoist the motion config lookup outside the
LazyColumn (call provideMotionConfig() once, e.g., val motionConfig =
provideMotionConfig() in SyncedLyricsSelector) and replace the integer division
expression line.frame / provideMotionConfig().fps with a floating-point division
such as line.frame.toFloat() / motionConfig.fps (and format the result to the
desired precision) so times like 15 frames at 30fps render as 0.5 sec instead of
0 sec.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt`:
- Around line 51-53: Make the three publicly visible vals into private
properties to hide implementation details: change okHttp, remote, and repository
in LyricsContainer (the vals named okHttp, remote, repository) to private val so
they are not exposed on the view's public API; ensure any internal usages within
the class still reference these renamed-private properties and adjust any
external references to use public methods instead.
- Around line 89-97: The code assumes songName contains " - " and directly
accesses songDetails[1], causing IndexOutOfBoundsException; update the logic in
LyricsContainer (the block using songName.split(" - "),
repository.getAlbumArtUrl(...), repository.getAlbumArtBitmap(...) and
setImageBitmap(...)) to safely handle cases where the delimiter is absent—either
check songName.contains(" - ") or split with a max of 2 parts and guard that the
resulting list has at least 2 elements before calling repository.getAlbumArtUrl,
and only proceed to getAlbumArtBitmap and setImageBitmap when both artist/title
parts are present.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt`:
- Around line 88-89: The getter for selectedSongName currently interpolates
selectedLyric.value?.trackName and ?.artistName producing "null - null" when
selectedLyric.value is null; update the selectedSongName getter to check
selectedLyric.value and return either an empty string or a placeholder (e.g.,
"") when null, otherwise format "${trackName} - ${artistName}" using the
non-null properties; locate the getter for selectedSongName and modify it to use
a null-check (or Elvis operator/let) against selectedLyric.value to avoid
emitting the literal "null - null".
In `@modules/motion-store/build.gradle`:
- Around line 1-4: The plugins block currently applies
alias(libs.plugins.android.library) and id 'maven-publish' but is missing the
Kotlin Android plugin; update the plugins block to include
alias(libs.plugins.kotlin.android) so Kotlin sources (e.g., DatabaseProvider.kt,
MotionProject.kt, ExampleUnitTest.kt) compile, ensuring the new alias is added
alongside alias(libs.plugins.android.library) and id 'maven-publish'.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/DatabaseProvider.kt`:
- Line 13: The database filename is misleadingly set to "firestore-offline.db"
even though this is a Nitrite DB; locate the
.filePath(context.getDatabasePath("firestore-offline.db")) call (in
DatabaseProvider.kt) and change the hardcoded filename to a clearer name such as
"motion-store.db" or "nitrite-motion-store.db" so it accurately reflects the DB
type; update any related tests or configuration that reference the old filename
to use the new name.
- Line 20: Replace the hardcoded credentials used when opening the Nitrite DB in
DatabaseProvider (the .openOrCreate("user", "password") call) by either calling
the no-credentials overload (openOrCreate()) or reading credentials from
configuration/environment (e.g., a Config/Environment variable injected into the
DatabaseProvider) and passing those values; update the DatabaseProvider
initialization to avoid inline "user"/"password" literals and validate presence
of configured credentials before using them.
- Around line 7-9: DatabaseProvider's private constructor prevents any
instantiation—add a companion object providing a factory/singleton accessor
(e.g., companion object with a `@Volatile` private INSTANCE and a
getInstance(context: Context) method) that initializes DatabaseProvider with
context.applicationContext in a thread-safe synchronized block (or use
double-checked locking) and returns the singleton; alternatively, if multiple
instances are intended, change the constructor visibility to public and add a
create(context: Context) factory function in the companion object to construct
new DatabaseProvider instances.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/MotionProject.kt`:
- Around line 19-28: The companion object's from function currently has no
return type or body and therefore does nothing; change its signature to return a
MotionProject and implement the body to construct and return a MotionProject
instance using the provided id, name, path, and by converting MotionConfig and
VideoAspectRatio into the stored fields expected by MotionProject (use existing
mappers/serializers or constructors for MotionConfig and VideoAspectRatio where
applicable). Specifically update companion object fun from(...) to return
MotionProject and populate all MotionProject properties from the passed
parameters (use methods on MotionConfig/VideoAspectRatio or existing conversion
helpers in the module to produce the stored representations).
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/BaseContourMotionView.kt`:
- Line 8: Remove the unused import for MotionConfig from BaseContourMotionView
(the top import "import com.tejpratapsingh.motionlib.core.MotionConfig"); simply
delete that import line in BaseContourMotionView.kt so the file no longer
references MotionConfig, then rebuild to ensure no other references exist.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/container/RotatingMotionView.kt`:
- Line 7: Remove the unused import of MotionVideoProducer from
RotatingMotionView (the import statement "import
com.tejpratapsingh.motionlib.core.motion.MotionVideoProducer") since it is never
referenced in the file; simply delete that import to resolve the unused-import
warning and keep imports minimal.
In
`@modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/ConfigParser.kt`:
- Around line 17-22: The JsonObject.toMotionConfig() method currently calls
get("aspectRatio").asJsonObject and get("fps").asInt /
get("outputQuality").asInt without null-safety; update this function to
defensively handle missing or malformed fields by using safe access and defaults
or by catching parse errors: e.g., check get("fps") and get("outputQuality") for
null (or use the safe-call operator and elvis defaults) and validate
get("aspectRatio") before calling toVideoAspectRatio(), or wrap the whole
parsing in a try/catch and return a fallback MotionConfig on error; ensure you
reference JsonObject.toMotionConfig, MotionConfig, and the get(...).asInt /
get(...).asJsonObject usages when making the changes.
🧹 Nitpick comments (21)
modules/pytorch-motion-ext/build.gradle (1)
23-29:compileOptionstargets Java 11 — confirm this is intentional given the JDK 21 project upgrade.The project-wide JDK was bumped to 21, but this module still targets
JavaVersion.VERSION_11for source/target compatibility. This is valid (compile for a lower target), but worth confirming it's deliberate and not an oversight.modules/motion-store/src/test/java/com/tejpratapsingh/motionstore/ExampleUnitTest.kt (1)
1-16: Boilerplate placeholder test — consider adding real unit tests for the motion-store module.This is the same auto-generated test present in every module. Given that motion-store introduces a Nitrite database layer (per the PR summary), meaningful tests for
DatabaseProviderandMotionProjectwould be valuable.modules/lyrics-maker/src/main/res/layout/lyrics_container.xml (1)
70-84:pb_progressstill referencesfake_chart_viewvia constraint, but both are effectively disconnected.
pb_progressis nowgone(line 81) and constrains its bottom tofake_chart_view(line 82), which itself no longer anchors tobtn_container. Sincepb_progressis invisible, there's no runtime issue, but if these elements are later toggled visible (e.g., via SDUI), the constraint chain betweenpb_progress → fake_chart_view → parentmay produce unexpected positioning. Worth a quick sanity check if visibility toggling is planned.gradle.properties (1)
23-31: Consider removingandroid.enableJetifier=true(line 19) during this build modernization.Jetifier has been deprecated since AGP 7.x and adds build overhead. Since you're upgrading to Gradle 9.1.0 and modernizing the build, this is a good opportunity to verify all transitive dependencies are AndroidX-native and drop it.
Also, the newly added flags appear to opt out of several Gradle 9 / AGP defaults (e.g.,
strictFullModeForKeepRules=false,optimizedResourceShrinking=false). These are reasonable for a safe migration, but consider revisiting them once the upgrade stabilizes to benefit from the improved defaults.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/di/OkHttpProvider.kt (1)
7-26: Consider adding awriteTimeoutand whether fully disabling connection pooling is intentional.You set
connectTimeoutandreadTimeoutbut nowriteTimeout— OkHttp defaults to 10s which is likely fine, but for consistency you may want to be explicit.The
ConnectionPool(0, ...)+Connection: closeheader fully disables connection reuse. This is fine for infrequent requests but will add latency (TCP+TLS handshake per request) if the client makes bursts of calls to the same host (e.g., MusicBrainz lookup followed by cover art fetch).modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/BaseFrameMotionView.kt (1)
83-84: CacheprovideMotionConfig()in a local variable to avoid redundant calls.
onMeasureis called frequently during layout passes. CallingprovideMotionConfig()twice is unnecessary when a single localvalsuffices.♻️ Suggested refactor
- val desiredWidth = provideMotionConfig().aspectRatio.width - val desiredHeight = provideMotionConfig().aspectRatio.height + val config = provideMotionConfig() + val desiredWidth = config.aspectRatio.width + val desiredHeight = config.aspectRatio.heightmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/background/GradientView.kt (1)
64-73: Same provider pattern — consider caching in a localval.Minor readability improvement: capture
provideMotionConfig()once instead of calling it twice.♻️ Suggested refactor
+ val config = provideMotionConfig() contourWidthOf { - provideMotionConfig() - .aspectRatio.width + config.aspectRatio.width .toXInt() } contourHeightOf { - provideMotionConfig() - .aspectRatio.height + config.aspectRatio.height .toYInt() }modules/3d-filament-renderer/src/main/java/com/tejpratapsingh/motionlib/filamentrenderer/Filament3dView.kt (1)
67-88: Six separateprovideMotionConfig()calls in this class — extract to a local or field.
provideMotionConfig()is called ininitializeFilament()(4 times) andsetupCamera()(2 times). Caching the config once at the call site improves readability and avoids any theoretical inconsistency if the backing store were updated between calls.♻️ Suggested refactor for initializeFilament
private fun initializeFilament() { Utils.init() + val config = provideMotionConfig() surfaceTexture = SurfaceTexture(0) surfaceTexture.setDefaultBufferSize( - provideMotionConfig().aspectRatio.width, - provideMotionConfig().aspectRatio.height, + config.aspectRatio.width, + config.aspectRatio.height, ) surface = Surface(surfaceTexture) engine = Engine.create() swapChain = engine.createSwapChain(surface, 0) scene = engine.createScene() view = engine.createView() view.scene = scene renderer = engine.createRenderer() view.viewport = Viewport( 0, 0, - provideMotionConfig().aspectRatio.width, - provideMotionConfig().aspectRatio.height, + config.aspectRatio.width, + config.aspectRatio.height, ) }modules/3d-opengl-renderer/src/main/java/com/tejpratapsingh/motionlib/openglrenderer/MotionOpenGlView.kt (1)
34-35:provideMotionConfig()called twice — risk of inconsistent width/height.If the backing config changes between the two calls,
widthandheightcould come from different configs. Store the result in a localvaland read both properties from it.Proposed fix
+ val motionConfig = provideMotionConfig() val offscreenRenderer = Object3DToBitmapRenderer( context = context, assetFileName = modelAssetPath, - width = provideMotionConfig().aspectRatio.width, - height = provideMotionConfig().aspectRatio.height, + width = motionConfig.aspectRatio.width, + height = motionConfig.aspectRatio.height, objectColor = floatArrayOf(0.7f, 0.3f, 0.3f, 1.0f), )modules/app/src/main/java/com/tejpratapsingh/animator/ui/view/RenaultCar.kt (1)
53-62: Same pattern: cacheprovideMotionConfig()in a localval.Two calls in the
initblock — store the result once for consistency and clarity, same as noted inMotionOpenGlView.kt.Proposed fix
+ val motionConfig = provideMotionConfig() contourHeightOf { - provideMotionConfig() - .aspectRatio.height + motionConfig + .aspectRatio.height .toYInt() } contourWidthOf { - provideMotionConfig() - .aspectRatio.width + motionConfig + .aspectRatio.width .toXInt() }modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/albumart/client/AlbumArtRemoteDataSource.kt (1)
5-13: Inverted dependency: data source depends on repository forCoverSize.Typically the repository depends on the data source, not vice versa. Having
AlbumArtRemoteDataSourcereferenceAlbumArtRepository.CoverSizeinverts this direction. Consider extractingCoverSizeinto its own top-level enum or a shared types file so neither layer depends on the other for type definitions.modules/ivi-demo/src/main/java/com/tejpratapsingh/ivi_demo/motion/RenaultCar.kt (1)
75-84: Consider caching the config to a local val.
provideMotionConfig()is called twice in the init block. If the provider ever becomes non-trivial, this could return different instances between calls. A singleval config = provideMotionConfig()would be more robust.Proposed fix
+ val config = provideMotionConfig() contourHeightOf { - provideMotionConfig() - .aspectRatio.height + config.aspectRatio.height .toYInt() } contourWidthOf { - provideMotionConfig() - .aspectRatio.width + config.aspectRatio.width .toXInt() }modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/VideoAspectRatioParser.kt (2)
46-63: NewGson/GsonBuilderinstances allocated on every call.Both
toJsonObject()andtoVideoAspectRatio()create newGsoninstances per invocation. If these are called frequently (e.g., in a list or loop), consider extracting the configuredGsonto a module-levelval.
26-44: Consider adding a comment or test to preventallRatiosfrom drifting.Currently, the
allRatioslist correctly includes all 16 non-Customsealed subtypes ofVideoAspectRatio. However, if a new subtype is added to the sealed class without updating this list, deserialization will silently fall back toCustom. A comment aboveallRatiosor a test validating completeness would prevent future maintenance issues.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/lrclib/model/LyricsResponse.kt (1)
45-54: Gson instances are created on everytoJson()/fromJson()call.Consider hoisting a shared
Gsoninstance into the companion object to avoid repeated construction.♻️ Proposed fix
companion object { + private val gson = Gson() + fun fromJson(json: String): LyricsResponse { - val gson = Gson() return gson.fromJson(json, LyricsResponse::class.java) } } + + fun toJson(): String = gson.toJson(this) + + companion object { + // move gson here + }modules/motion-store/src/androidTest/java/com/tejpratapsingh/motionstore/ExampleInstrumentedTest.kt (1)
1-22: Boilerplate test — consider adding actual tests forDatabaseProvider.This is the default Android Studio template. Given that
DatabaseProviderhandles database initialization with credentials and file paths, it would benefit from dedicated tests verifying open/close lifecycle and data persistence.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/lrclib/client/LyricsRepositoryImpl.kt (1)
10-15: Pure pass-through toapiService— consider if this layer adds value now.Currently this repository is a 1:1 delegation with no caching, mapping, or error enrichment. It's fine as a structural placeholder, but if no additional logic is planned (e.g., caching, offline support), this could be simplified.
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/lrclib/client/LrcLibApiServiceImpl.kt (1)
24-69: Significant code duplication betweensearchandget.Both methods share the same pattern: build URL → construct request → execute → check success → read body → parse JSON. Consider extracting a private helper to reduce duplication.
Also applies to: 71-115
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SearchLyricsCompose.kt (1)
96-144: Extract local variable to avoid repeatedstate.lyrics[item]indexing.
state.lyrics[item]is accessed five times in the loop body. A localval lyric = state.lyrics[item]at the top of the lambda improves readability and avoids repeated list indexing.Suggested refactor
items(state.lyrics.size) { item -> + val lyric = state.lyrics[item] Card( modifier = Modifier .fillMaxWidth() .padding(vertical = 4.dp) - .clickable { onLyricsSelected(state.lyrics[item]) }, + .clickable { onLyricsSelected(lyric) }, // ... ) { Text( - text = "${state.lyrics[item].trackName} - ${state.lyrics[item].artistName}", + text = "${lyric.trackName} - ${lyric.artistName}", // ... ) Text( - text = "Duration: ${state.lyrics[item].getReadableDuration()}", + text = "Duration: ${lyric.getReadableDuration()}", // ... ) Text( - text = state.lyrics[item].getLyrics(), + text = lyric.getLyrics(), // ... ) } }modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt (2)
57-86: Concurrent searches can race — later query may be overwritten by an earlier response.If the user triggers
searchLyricsrapidly (e.g., clicking multiple recent searches), each call launches independently inviewModelScope. A slow first request completing after a fast second request would overwrite the UI state with stale results.Cancel the previous job before launching a new one:
Proposed fix
class LyricsViewModel : ViewModel() { // ... existing fields ... + private var searchJob: Job? = null fun searchLyrics( trackName: String? = null, artistName: String? = null, albumName: String? = null, query: String? = null, ) { - viewModelScope.launch { + searchJob?.cancel() + searchJob = viewModelScope.launch { _uiState.value = LyricsUiState.Loading // ... } }
42-46: Manual wiring of dependencies inside the ViewModel.Constructing
OkHttpClient,Gson,LrcLibApiServiceImpl, andLyricsRepositoryImpldirectly in the ViewModel couples it to concrete implementations and makes testing harder. Consider injectingLyricsRepositoryvia the constructor (e.g., using aViewModelProvider.Factoryor Hilt) so the ViewModel stays testable and decoupled.
| <project version="4"> | ||
| <component name="CompilerConfiguration"> | ||
| <bytecodeTargetLevel target="17" /> | ||
| <bytecodeTargetLevel target="21" /> |
There was a problem hiding this comment.
IDE bytecode target (21) is inconsistent with module-level compileOptions (Java 11).
All module build.gradle files in this PR still set sourceCompatibility/targetCompatibility to JavaVersion.VERSION_11. Gradle controls the actual build, so this IDE setting won't affect artifacts, but the mismatch can cause confusing IDE-vs-CLI compilation differences. Consider aligning these — either bump compileOptions to Java 21 across modules, or keep this at 11 to match.
🤖 Prompt for AI Agents
In @.idea/compiler.xml at line 4, The IDE bytecodeTargetLevel is set to "21" but
module compileOptions/sourceCompatibility and targetCompatibility are still
JavaVersion.VERSION_11; update one side so they match. Either change the
<bytecodeTargetLevel target="21"> entry to target="11" to match existing
compileOptions/sourceCompatibility and targetCompatibility, or update all module
build.gradle settings (sourceCompatibility/targetCompatibility and any
compileOptions.release) to Java 21 so Gradle and the IDE are consistent; adjust
the corresponding symbol(s) bytecodeTargetLevel and the Gradle properties
sourceCompatibility/targetCompatibility/compileOptions.release accordingly.
...ament-renderer/src/main/java/com/tejpratapsingh/motionlib/filamentrenderer/Filament3dView.kt
Outdated
Show resolved
Hide resolved
modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionConfig.kt
Show resolved
Hide resolved
modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionConfig.kt
Outdated
Show resolved
Hide resolved
...java/com/tejpratapsingh/lyricsmaker/data/api/albumart/client/AlbumArtRemoteDataSourceImpl.kt
Outdated
Show resolved
Hide resolved
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/DatabaseProvider.kt
Outdated
Show resolved
Hide resolved
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/MotionProject.kt
Outdated
Show resolved
Hide resolved
...es/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/BaseContourMotionView.kt
Outdated
Show resolved
Hide resolved
...tionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/container/RotatingMotionView.kt
Outdated
Show resolved
Hide resolved
modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/ConfigParser.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt (1)
79-100:⚠️ Potential issue | 🔴 Critical
runBlockingin a View'sinitblock will freeze the UI thread (ANR risk).
runBlockingon the main thread performs synchronous network I/O (MusicBrainz lookup + image download) during view construction. On Android, this will block the UI thread and likely trigger an Application Not Responding (ANR) dialog. Use a coroutine scope (e.g.,lifecycleScopeor a passed-inCoroutineScope) withlaunchinstead, and set the image asynchronously with a placeholder or visibility toggle.modules/3d-opengl-renderer/build.gradle (1)
1-4:⚠️ Potential issue | 🔴 CriticalRestore the
kotlin.androidplugin — this module contains Kotlin source files and will fail to compile without it.The
kotlin.androidplugin was removed but the module has four Kotlin source files (MotionOpenGlView.kt,ObjModel.kt,Object3DToBitmapRenderer.kt,OffscreenRenderer.kt) that require the plugin to be compiled. Removing it while retaininglibs.androidx.core.ktxas a dependency will cause a build failure.modules/jcodec-motion-ext/build.gradle (1)
1-4:⚠️ Potential issue | 🔴 CriticalThe Kotlin Android plugin must not be removed—the module contains Kotlin source files.
The
kotlin.androidplugin alias was removed, but this module has three Kotlin source files that require it to compile:
modules/jcodec-motion-ext/src/main/java/com/tejpratapsingh/motionlib/jcodec/FileExtension.ktmodules/jcodec-motion-ext/src/main/java/com/tejpratapsingh/motionlib/jcodec/JCodecVideoProducerAdapter.ktmodules/jcodec-motion-ext/src/test/java/com/tejpratapsingh/motionlib/jcodec/ExampleUnitTest.ktWithout the Kotlin plugin, the build will fail. Restore the
alias(libs.plugins.kotlin.android)line.modules/motionlib/build.gradle (1)
1-4:⚠️ Potential issue | 🔴 CriticalAdd Kotlin plugin back to motionlib/build.gradle—this module has 17+ Kotlin source files and will fail to compile without it.
The
kotlin-androidplugin was removed from this module, but it contains Kotlin sources (MotionComposerView.kt,BaseFrameMotionView.kt,BaseContourMotionView.kt,MotionVideoProducer.kt, and many others). The Kotlin plugin is not applied globally (neither insettings.gradlenor in the rootbuild.gradle), so without it, Gradle cannot compile the.ktfiles and the build will fail.Restore
alias(libs.plugins.kotlin.android)in thepluginsblock.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/LyricsActivity.kt (1)
69-70:⚠️ Potential issue | 🟠 Major
minBy/maxByon an emptylyricslist will throwNoSuchElementException.The
lyricsproperty falls back toemptyList()when the intent extra is missing. If that happens,minByandmaxBycrash. Add an early return or guard.Proposed fix
override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) + if (lyrics.isEmpty()) { + finish() + return + } + val start = lyrics.minBy { it.frame }.frame val end = lyrics.maxBy { it.frame }.framemodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt (1)
103-113:⚠️ Potential issue | 🔴 Critical
field.first()will throwNoSuchElementExceptionon an empty list.The backing field defaults to
emptyList()(line 103). Any access toselectedLyricsbefore it's reassigned to a non-empty list will crash. UsefirstOrNull()with an early return, or guard against the empty case.Proposed fix
var selectedLyrics: List<SyncedLyricFrame> = emptyList() get() { + if (field.isEmpty()) return emptyList() val firstFrame = field.first().frame return field .map {
🤖 Fix all issues with AI agents
In `@gradle/libs.versions.toml`:
- Around line 21-22: The Kotlin version in libs.versions.toml is pinned to
kotlinVersion = "2.3.0", which is not officially compatible with agpVersion =
"9.0.0"; update kotlinVersion to a compatible release (e.g., "2.3.10" or later)
so Kotlin officially supports AGP 9.0.0, ensuring the kotlinVersion entry is
changed accordingly and any build files referencing kotlinVersion pick up the
new value.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/albumart/client/AlbumArtRemoteDataSourceImpl.kt`:
- Around line 63-73: The fetchBitmap implementation can throw IOException when
calling client.newCall(request).execute(), so wrap the network call inside a
try/catch in the withContext(Dispatchers.IO) block (around
client.newCall(request).execute().use { ... }) to catch IOException (and
optionally other exceptions) and return null on error; keep the existing null
returns for non-successful responses and decoding failures, and optionally log
the exception for debugging in the catch block.
- Around line 44-60: Wrap the network call and JSON parsing inside a try-catch
in the withContext block so IO and serialization exceptions are caught and the
method returns null instead of throwing; specifically surround
client.newCall(request).execute() and
json.decodeFromString<MusicBrainzResponse>(bodyString) with a try { ... } catch
(e: Exception) { /* optionally log e */ return@withContext null }, keeping the
existing success path that builds the cover art URL (the code in
AlbumArtRemoteDataSourceImpl that reads response.body()?.string(), parses into
MusicBrainzResponse, extracts releaseId and returns the URL should remain inside
the try block).
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SyncedLyricsSelector.kt`:
- Line 38: Remove the unused import of MotionVideoProducer from
SyncedLyricsSelector.kt; locate the import statement "import
com.tejpratapsingh.motionlib.core.motion.MotionVideoProducer" at the top of the
file and delete it so the file no longer contains an unused reference to
MotionVideoProducer.
In
`@modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/VideoAspectRatioParser.kt`:
- Around line 53-60: The deserializer currently calls obj.get("width").asInt /
obj.get("height").asInt / obj.get("label").asString without guarding for missing
keys which causes NPEs; update the JsonDeserializer block to defensively parse
these fields: use obj.has("width")/obj.has("height")/obj.has("label") (or check
obj.get(...) != null) before calling asInt/asString, throw a JsonParseException
or provide sensible defaults if a required field is absent, and then continue to
lookup in allRatios or construct Custom(width, height, label) — reference the
JsonDeserializer lambda, the element variable, allRatios.find { it.width ==
width && it.height == height }, and the Custom(width, height, label) constructor
when making the change.
🧹 Nitpick comments (14)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt (1)
82-86: Inconsistent HTTP clients: KtorHttpClient(CIO)here vs. OkHttp via repository in theelsebranch.The
image != nullpath creates a one-off KtorHttpClientfor fetching the bitmap, while theelsebranch uses the OkHttp-backed repository. Consider usingrepository.getAlbumArtBitmap(image)(or the OkHttp-based remote) for consistency and to avoid pulling in two HTTP client engines.♻️ Proposed fix
if (image != null) { - val client = HttpClient(CIO) Log.i(TAG, "Using image from social meta: $image") - setImageBitmap(client.fetchBitmap(image)) - client.close() - return@runBlocking + repository.getAlbumArtBitmap(image)?.also { + setImageBitmap(it) + } } else {gradle/wrapper/gradle-wrapper.properties (1)
3-3: Consider upgrading to Gradle 9.3.1 (latest stable) for improved performance and stability.Gradle 9.1.0 is valid, but 9.3.1 is the latest stable release available (released January 29, 2026). Gradle 9.2.0+ introduced several performance and stability improvements over 9.1.0.
Since this is a major version jump (8.13 → 9.1.0), be aware that Gradle 9.0.0+ requires Java 17+ to run and uses Kotlin 2.2.0 and Groovy 4, which can introduce breaking changes. However, your codebase is already configured with AGP 9.0.0 and Kotlin 2.3.0, which are compatible with Gradle 9.x, so no plugin compatibility issues are expected.
modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/BaseFrameMotionView.kt (1)
83-84: CacheprovideMotionConfig()to avoid redundant calls inonMeasure.
onMeasureis called frequently during layout passes. CallingprovideMotionConfig()twice risks both unnecessary overhead and a (theoretical) inconsistency if the backing config changes between calls.♻️ Proposed fix
override fun onMeasure( widthMeasureSpec: Int, heightMeasureSpec: Int, ) { - val desiredWidth = provideMotionConfig().aspectRatio.width - val desiredHeight = provideMotionConfig().aspectRatio.height + val config = provideMotionConfig() + val desiredWidth = config.aspectRatio.width + val desiredHeight = config.aspectRatio.height setMeasuredDimension(desiredWidth, desiredHeight)modules/ivi-demo/src/main/java/com/tejpratapsingh/ivi_demo/motion/RenaultCar.kt (1)
12-13:MotionVideoProducerimport appears unused.Line 12 imports
MotionVideoProducer, but it is not referenced anywhere in this file. This may be a leftover from a refactoring step.🧹 Remove unused import
-import com.tejpratapsingh.motionlib.core.motion.MotionVideoProducer import com.tejpratapsingh.motionlib.core.provideMotionConfigmodules/3d-opengl-renderer/src/main/java/com/tejpratapsingh/motionlib/openglrenderer/MotionOpenGlView.kt (2)
30-37: CallprovideMotionConfig()once to avoid inconsistency and redundant lookups.If the global config were ever swapped between the two calls,
widthandheightcould come from different configs. Store the result in a local:♻️ Proposed fix
+ private val motionConfig = provideMotionConfig() + val offscreenRenderer = Object3DToBitmapRenderer( context = context, assetFileName = modelAssetPath, - width = provideMotionConfig().aspectRatio.width, - height = provideMotionConfig().aspectRatio.height, + width = motionConfig.aspectRatio.width, + height = motionConfig.aspectRatio.height, objectColor = floatArrayOf(0.7f, 0.3f, 0.3f, 1.0f), )
10-10: Unused import:MotionVideoProduceris not referenced in this file.This import appears to be leftover from another change. Consider removing it to keep the file clean.
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/DatabaseProvider.kt (1)
16-21:dbis publicly exposed as a mutableNitriteinstance — breaks encapsulation.The
val dbproperty is public, allowing any consumer to directly manipulate the database (drop collections, close it, etc.) bypassingDatabaseProvider's ownclose()method. Consider making itprivateorinternaland exposing only specific repository/collection accessors.♻️ Proposed fix
- val db = + private val db = Nitrite .builder() .loadModule(storeModule) - .openOrCreate("user", "password") + .openOrCreate()modules/motionlib/build.gradle (1)
22-25:VERSION_1_8is inconsistent with other modules usingVERSION_11.Other modules in this PR (e.g.,
tensorflow-motion-ext,3d-filament-renderer) useJavaVersion.VERSION_11. This module still targets Java 1.8. While this isn't a new change, it's worth aligning for consistency, especially if downstream modules depend on Java 11 features.modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/VideoAspectRatioParser.kt (2)
46-46: Gson instances are created on every call — consider reusing a shared instance.Both
toJsonObject()andtoVideoAspectRatio()allocate newGson/GsonBuilderinstances per invocation. For a utility that may be called repeatedly (e.g., parsing a list of configs), this adds unnecessary GC pressure.♻️ Proposed fix — hoist Gson instances to file-level
+private val gson = Gson() + +private val videoAspectRatioGson: Gson = + GsonBuilder() + .registerTypeAdapter( + VideoAspectRatio::class.java, + JsonDeserializer { element, _, _ -> + val obj = element.asJsonObject + val width = obj.get("width")?.asInt + ?: throw IllegalArgumentException("Missing 'width'") + val height = obj.get("height")?.asInt + ?: throw IllegalArgumentException("Missing 'height'") + val label = obj.get("label")?.asString ?: "${width}x${height}" + allRatios.find { it.width == width && it.height == height } + ?: Custom(width, height, label) + }, + ).create() + -fun VideoAspectRatio.toJsonObject(): JsonObject = Gson().toJsonTree(this).asJsonObject +fun VideoAspectRatio.toJsonObject(): JsonObject = gson.toJsonTree(this).asJsonObject -fun JsonObject.toVideoAspectRatio(): VideoAspectRatio { - val gson = - GsonBuilder() - ... - return gson.fromJson(this, VideoAspectRatio::class.java) -} +fun JsonObject.toVideoAspectRatio(): VideoAspectRatio = + videoAspectRatioGson.fromJson(this, VideoAspectRatio::class.java)Also applies to: 48-63
26-44: Consider automating ratio discovery to prevent maintenance drift.
VideoAspectRatiois a sealed class, and all 16 predefined ratios are currently inallRatios. However, if a new ratio is added toVideoAspectRatioin the future but forgotten inallRatios, deserialization silently falls back toCustom. Since Kotlin sealed classes support runtime introspection, consider usingVideoAspectRatio::class.sealedSubclassesor reflection to auto-discover predefined ratios instead of maintaining this list manually.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/lrclib/model/LyricsResponse.kt (1)
7-7: Unused import:kotlinx.serialization.Serializable.The
@Serializableannotation was removed from the class but its import remains. This is dead code.Proposed fix
-import kotlinx.serialization.Serializablemodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt (1)
59-63: Extract duplicated fallback expression.
it.title ?: it.description ?: ""is repeated on lines 61 and 62. Extract it to a local variable for clarity and to avoid divergence if the fallback logic changes.Proposed fix
ShareReceiverActivity.readMetadataFromIntent(intent)?.let { lyricsViewModel.socialMeta.value = it - lyricsViewModel.query.value = it.title ?: it.description ?: "" - lyricsViewModel.searchLyrics(it.title ?: it.description ?: "") + val searchQuery = it.title ?: it.description ?: "" + lyricsViewModel.query.value = searchQuery + lyricsViewModel.searchLyrics(query = searchQuery) }modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt (2)
50-52: Consider exposingquery,socialMeta, andselectedLyricas read-onlyStateFlowpublicly.Exposing
MutableStateFlowdirectly allows the View layer to mutate ViewModel state without going through defined functions, which undermines unidirectional data flow. Consider backing these with privateMutableStateFlowand exposing publicStateFlow(similar touiState), with dedicated update methods.
42-46: Manual service/repository wiring in the ViewModel constructor body.Instantiating
OkHttpProvider.httpClient,Gson(),LrcLibApiServiceImpl, andLyricsRepositoryImpldirectly couples the ViewModel to concrete implementations, making it harder to test. This is fine for now, but consider constructor injection (e.g., via aViewModelFactoryor Hilt) as the module grows.
...java/com/tejpratapsingh/lyricsmaker/data/api/albumart/client/AlbumArtRemoteDataSourceImpl.kt
Outdated
Show resolved
Hide resolved
| override suspend fun fetchBitmap(url: String): Bitmap? = | ||
| withContext(Dispatchers.IO) { | ||
| val request = Request.Builder().url(url).build() | ||
|
|
||
| client.newCall(request).execute().use { response -> | ||
| if (!response.isSuccessful) return@withContext null | ||
|
|
||
| val bytes = response.body()?.bytes() ?: return@withContext null | ||
| BitmapFactory.decodeByteArray(bytes, 0, bytes.size) | ||
| } | ||
| } |
There was a problem hiding this comment.
Same missing exception handling in fetchBitmap.
execute() can throw IOException here as well, crashing instead of returning null.
🐛 Proposed fix
override suspend fun fetchBitmap(url: String): Bitmap? =
withContext(Dispatchers.IO) {
- val request = Request.Builder().url(url).build()
-
- client.newCall(request).execute().use { response ->
- if (!response.isSuccessful) return@withContext null
-
- val bytes = response.body()?.bytes() ?: return@withContext null
- BitmapFactory.decodeByteArray(bytes, 0, bytes.size)
- }
+ try {
+ val request = Request.Builder().url(url).build()
+ client.newCall(request).execute().use { response ->
+ if (!response.isSuccessful) return@withContext null
+ val bytes = response.body()?.bytes() ?: return@withContext null
+ BitmapFactory.decodeByteArray(bytes, 0, bytes.size)
+ }
+ } catch (e: Exception) {
+ Log.e("AlbumArtRemoteDataSource", "fetchBitmap failed", e)
+ null
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override suspend fun fetchBitmap(url: String): Bitmap? = | |
| withContext(Dispatchers.IO) { | |
| val request = Request.Builder().url(url).build() | |
| client.newCall(request).execute().use { response -> | |
| if (!response.isSuccessful) return@withContext null | |
| val bytes = response.body()?.bytes() ?: return@withContext null | |
| BitmapFactory.decodeByteArray(bytes, 0, bytes.size) | |
| } | |
| } | |
| override suspend fun fetchBitmap(url: String): Bitmap? = | |
| withContext(Dispatchers.IO) { | |
| try { | |
| val request = Request.Builder().url(url).build() | |
| client.newCall(request).execute().use { response -> | |
| if (!response.isSuccessful) return@withContext null | |
| val bytes = response.body()?.bytes() ?: return@withContext null | |
| BitmapFactory.decodeByteArray(bytes, 0, bytes.size) | |
| } | |
| } catch (e: Exception) { | |
| Log.e("AlbumArtRemoteDataSource", "fetchBitmap failed", e) | |
| null | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/albumart/client/AlbumArtRemoteDataSourceImpl.kt`
around lines 63 - 73, The fetchBitmap implementation can throw IOException when
calling client.newCall(request).execute(), so wrap the network call inside a
try/catch in the withContext(Dispatchers.IO) block (around
client.newCall(request).execute().use { ... }) to catch IOException (and
optionally other exceptions) and return null on error; keep the existing null
returns for non-successful responses and decoding failures, and optionally log
the exception for debugging in the catch block.
...er/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SyncedLyricsSelector.kt
Outdated
Show resolved
Hide resolved
modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/VideoAspectRatioParser.kt
Show resolved
Hide resolved
tejpratap46
force-pushed
the
feat/implement-sdui
branch
from
3a9034b to
0be5ebd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/LyricsActivity.kt (1)
69-70:⚠️ Potential issue | 🟠 Major
minBy/maxBywill throwNoSuchElementExceptioniflyricsis empty.
lyricscan be an empty list (the getter defaults toemptyList()). If the activity is started without valid lyrics data, these calls crash immediately inonCreate.Consider guarding with an early return or using
minByOrNull/maxByOrNull.🐛 Proposed fix
- val start = lyrics.minBy { it.frame }.frame - val end = lyrics.maxBy { it.frame }.frame + if (lyrics.isEmpty()) { + finish() + return + } + val start = lyrics.minOf { it.frame } + val end = lyrics.maxOf { it.frame }modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt (1)
79-100:⚠️ Potential issue | 🟠 Major
runBlockingininitperforms network I/O on the calling thread, likely the main/UI thread.This will freeze the UI during album art fetching and can easily trigger an ANR. While the
runBlockingwrapper pre-dates this PR, the newly introducedrepository.getAlbumArtUrlandrepository.getAlbumArtBitmapcalls are suspend functions performing network I/O that will block here. Consider moving this to a coroutine scope (e.g.,lifecycleScopeor aCoroutineScopepassed into the view) so the network calls happen off the main thread.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt (1)
103-113:⚠️ Potential issue | 🔴 Critical
selectedLyricsgetter crashes withNoSuchElementExceptionwhen the backing list is empty.
fielddefaults toemptyList(), and the getter unconditionally callsfield.first()on Line 105. Any access before a non-empty list is assigned (or after assigning an empty list) will crash at runtime.Proposed fix
var selectedLyrics: List<SyncedLyricFrame> = emptyList() get() { + if (field.isEmpty()) return emptyList() val firstFrame = field.first().frame return field .map { SyncedLyricFrame( frame = it.frame - firstFrame, text = it.text, ) }.sortedBy { it.frame } }
🤖 Fix all issues with AI agents
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/lrclib/model/LyricsResponse.kt`:
- Line 7: Remove the unused import kotlinx.serialization.Serializable from
LyricsResponse.kt; locate the import statement for
kotlinx.serialization.Serializable (the leftover after removing the
`@Serializable` annotation from the LyricsResponse class) and delete that import
so there are no unused imports in the file.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SearchLyricsCompose.kt`:
- Around line 148-179: The current when-block only handles LyricsUiState.Success
and uses the else branch to render recentSearches, which swallows
LyricsUiState.Error; add an explicit branch for LyricsUiState.Error (matching
the sealed/state class used) that displays the error message from the error
state (e.g., state.message) and a retry affordance that calls
viewModel.searchLyrics(...) or re-emits the query (using viewModel.query.tryEmit
or the existing clickable handler), while keeping the recentSearches rendering
for the Initial/Loading branches or adding separate Loading UI; update the when
to handle LyricsUiState.Error, LyricsUiState.Loading and LyricsUiState.Initial
explicitly instead of using the catch-all else so users see errors and can
retry.
🧹 Nitpick comments (10)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt (1)
59-63: Extract duplicated fallback expression into a local variable.
it.title ?: it.description ?: ""is repeated on lines 61 and 62. A localvalavoids the duplication and makes future changes less error-prone.♻️ Suggested refactor
ShareReceiverActivity.readMetadataFromIntent(intent)?.let { lyricsViewModel.socialMeta.value = it - lyricsViewModel.query.value = it.title ?: it.description ?: "" - lyricsViewModel.searchLyrics(it.title ?: it.description ?: "") + val searchQuery = it.title ?: it.description ?: "" + lyricsViewModel.query.value = searchQuery + lyricsViewModel.searchLyrics(searchQuery) }modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SearchLyricsCompose.kt (1)
96-142: Extract item to a local variable and provide a stable key toitems.
state.lyrics[item]is dereferenced five times inside the lambda. Pulling it into a localvalimproves readability and avoids redundant list lookups. Additionally,LyricsResponsehas a stableidfield that should be used as the key for theitemscomposable, allowing Compose to correctly diff the list when contents change and prevent stale recompositions.♻️ Proposed refactor
- items(state.lyrics.size) { item -> + items(state.lyrics.size, key = { state.lyrics[it].id }) { item -> + val lyrics = state.lyrics[item] Card( modifier = Modifier .fillMaxWidth() .padding(vertical = 4.dp) - .clickable { onLyricsSelected(state.lyrics[item]) }, + .clickable { onLyricsSelected(lyrics) }, colors = CardDefaults.cardColors( containerColor = MaterialTheme.colorScheme.surfaceVariant, ), ) { Text( - text = "${state.lyrics[item].trackName} - ${state.lyrics[item].artistName}", + text = "${lyrics.trackName} - ${lyrics.artistName}", style = MaterialTheme.typography.labelLarge, modifier = Modifier.padding( start = 16.dp, top = 16.dp, end = 16.dp, bottom = 2.dp, ), ) Text( - text = "Duration: ${state.lyrics[item].getReadableDuration()}", + text = "Duration: ${lyrics.getReadableDuration()}", maxLines = 2, style = MaterialTheme.typography.bodyMedium, modifier = Modifier.padding( start = 16.dp, top = 2.dp, end = 16.dp, bottom = 2.dp, ), ) Text( - text = state.lyrics[item].getLyrics(), + text = lyrics.getLyrics(), maxLines = 2, style = MaterialTheme.typography.bodyMedium, modifier = Modifier.padding( start = 16.dp, top = 2.dp, end = 16.dp, bottom = 16.dp, ), ) Spacer(Modifier.height(4.dp)) }modules/3d-opengl-renderer/src/main/java/com/tejpratapsingh/motionlib/openglrenderer/MotionOpenGlView.kt (1)
34-35: Consider caching the config in a local variable to avoid redundant lookups.
provideMotionConfig()is called twice in succession. While likely cheap, a singleval config = provideMotionConfig()would be clearer and avoid any risk of the two calls returning different instances if the store is mutated concurrently.♻️ Suggested change
+ val motionConfig = provideMotionConfig() val offscreenRenderer = Object3DToBitmapRenderer( context = context, assetFileName = modelAssetPath, - width = provideMotionConfig().aspectRatio.width, - height = provideMotionConfig().aspectRatio.height, + width = motionConfig.aspectRatio.width, + height = motionConfig.aspectRatio.height, objectColor = floatArrayOf(0.7f, 0.3f, 0.3f, 1.0f), )modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/ConfigParser.kt (1)
7-15: Serialization creates a newGson()instance on every call.Consider extracting a shared
Gsoninstance (e.g., acompanion objector top-levelval) to avoid allocating a new serializer per invocation. The same applies toVideoAspectRatioParser.kt.modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/VideoAspectRatioParser.kt (2)
48-63: A newGsonBuilder+ custom deserializer is allocated on every call.
toVideoAspectRatio()builds a fully configuredGsoninstance each invocation. Extract theGsoninstance to a top-level or companionvalso it's created once.♻️ Suggested refactor
+private val videoAspectRatioGson: Gson = + GsonBuilder() + .registerTypeAdapter( + VideoAspectRatio::class.java, + JsonDeserializer { element, _, _ -> + val obj = element.asJsonObject + val width = obj.get("width")?.asInt + ?: throw IllegalArgumentException("Missing 'width'") + val height = obj.get("height")?.asInt + ?: throw IllegalArgumentException("Missing 'height'") + val label = obj.get("label")?.asString ?: "${width}x${height}" + allRatios.find { it.width == width && it.height == height } + ?: Custom(width, height, label) + }, + ).create() + fun JsonObject.toVideoAspectRatio(): VideoAspectRatio { - val gson = - GsonBuilder() - .registerTypeAdapter( - VideoAspectRatio::class.java, - JsonDeserializer { element, _, _ -> - val obj = element.asJsonObject - val width = obj.get("width").asInt - val height = obj.get("height").asInt - val label = obj.get("label").asString - - allRatios.find { it.width == width && it.height == height } - ?: Custom(width, height, label) - }, - ).create() - return gson.fromJson(this, VideoAspectRatio::class.java) + return videoAspectRatioGson.fromJson(this, VideoAspectRatio::class.java) }This also addresses the null-safety issue inline.
26-44: Use sealed-class reflection to auto-discover predefined ratios instead of manual list maintenance.Currently
allRatiosis manually kept in sync withVideoAspectRatiosubclasses. SinceVideoAspectRatiois a sealed class, you can usesealedSubclassesto auto-discover all predefined ratio objects and filter out only the data objects. This eliminates the risk of silently falling back toCustomwhen a new ratio is added to the sealed class but not appended toallRatios.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt (2)
50-52: Consider exposingquery,socialMeta, andselectedLyricasStateFlowwith private mutable backing fields.Exposing
MutableStateFlowdirectly allows any consumer to modify state, bypassing the ViewModel's control. The standard pattern uses a private_field/ publicfieldpair (as you already do for_uiState/uiState).
43-46: Consider injecting dependencies instead of creating them inline.Instantiating
OkHttpProvider.httpClient,Gson,LrcLibApiServiceImpl, andLyricsRepositoryImpldirectly in the ViewModel makes it difficult to unit test (can't substitute mocks). AViewModelFactoryor a DI framework (Hilt/Koin) would improve testability.gradle.properties (2)
23-31: Several AGP 9.0 defaults are explicitly disabled — document the rationale.Properties like
android.r8.strictFullModeForKeepRules=false,android.uniquePackageNames=false, andandroid.nonFinalResIds=falseopt out of AGP 9.0's new defaults. This is a valid incremental migration strategy, but these should be tracked as tech debt to enable over time, as they represent best-practice improvements (e.g., strict R8 keep rules, unique package names).Consider adding inline comments explaining why each is disabled.
19-19: Removeandroid.enableJetifier=trueto improve build performance.The codebase contains no old
android.support.*libraries—all dependencies are already fully migrated to AndroidX. Jetifier is unnecessary and disabling it eliminates runtime overhead on every build. While still supported in AGP 9.0, Google recommends removing it when not needed.
| import com.google.gson.Gson | ||
| import com.google.gson.annotations.SerializedName | ||
| import kotlinx.parcelize.Parcelize | ||
| import kotlinx.serialization.Serializable |
There was a problem hiding this comment.
Unused import: kotlinx.serialization.Serializable
The @Serializable annotation was removed from the class, but the import remains on line 7.
Proposed fix
-import kotlinx.serialization.Serializable📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import kotlinx.serialization.Serializable |
🤖 Prompt for AI Agents
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/lrclib/model/LyricsResponse.kt`
at line 7, Remove the unused import kotlinx.serialization.Serializable from
LyricsResponse.kt; locate the import statement for
kotlinx.serialization.Serializable (the leftover after removing the
`@Serializable` annotation from the LyricsResponse class) and delete that import
so there are no unused imports in the file.
...ker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SearchLyricsCompose.kt
Show resolved
Hide resolved
tejpratap46
force-pushed
the
feat/implement-sdui
branch
from
0be5ebd to
3021260
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
modules/lyrics-maker/build.gradle (1)
1-6:⚠️ Potential issue | 🔴 CriticalMissing
kotlin.androidplugin — this module will not build.This application module uses
kotlin.serialization,kotlin.parcelize, andkotlin.compose— all of which require the Kotlin Android plugin as a prerequisite. Without it, Kotlin sources won't compile and these plugins will fail to apply correctly.Proposed fix: restore the Kotlin Android plugin
plugins { alias(libs.plugins.android.application) + alias(libs.plugins.kotlin.android) alias(libs.plugins.kotlin.serialization) alias(libs.plugins.kotlin.parcelize) alias(libs.plugins.kotlin.compose) }modules/tensorflow-motion-ext/build.gradle (1)
1-4:⚠️ Potential issue | 🔴 CriticalRestore
kotlin.androidplugin — module contains Kotlin source files.The module has 7 Kotlin files (ImageUtils.kt, TensorFlowImageProcessor.kt, CarBgRemover.kt, TfLiteSegmentationHelper.kt, TiledBackgroundRemover.kt, SuperResolutionProcessor.kt, and ExampleUnitTest.kt). Without the
kotlin.androidplugin, Gradle will fail to compile these sources.modules/ffmpeg-motion-ext/build.gradle (1)
1-4:⚠️ Potential issue | 🔴 CriticalKotlin source files exist but
kotlin.androidplugin was removed.The module contains 4 Kotlin files (FfmpegVideoProducerAdapter.kt, FFMpegExtensions.kt, FFMpegVideoFrameView.kt, ExampleUnitTest.kt) but the
kotlin.androidplugin has been removed from the build configuration. Either restore thekotlin.androidplugin or migrate the Kotlin files to Java.modules/jcodec-motion-ext/build.gradle (1)
1-4:⚠️ Potential issue | 🔴 CriticalRestore the
kotlin.androidplugin—this module contains Kotlin source files.The module has 3 Kotlin files (
FileExtension.kt,JCodecVideoProducerAdapter.kt, andExampleUnitTest.kt) that will fail to compile without thekotlin-androidplugin. Removing it while retaining Kotlin sources will break the build.modules/3d-filament-renderer/build.gradle (1)
1-4:⚠️ Potential issue | 🔴 CriticalAdd
kotlin.androidplugin — Kotlin source files exist in this module.The module contains 3 Kotlin source files (Filament3dView.kt, FilamentOffscreenCapturer.kt, ExampleUnitTest.kt) but the
kotlin.androidplugin is missing. This will cause compilation failures.modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/infra/AndroidVideoGenerator.kt (1)
52-53:⚠️ Potential issue | 🟡 MinorStale
@param motionConfigin KDoc — parameter was removed.The
motionConfigparameter no longer exists in the method signature (it's now fetched internally viaprovideCurrentConfig()), but the KDoc still documents it. Remove or update this entry.📝 Proposed fix
- * `@param` motionConfig A [MotionConfig] object specifying video properties like - * width, height, and frames per second (fps).modules/ffmpeg-motion-ext/src/main/java/com/tejpratapsingh/motionlib/ffmpeg/FfmpegVideoProducerAdapter.kt (1)
148-151:⚠️ Potential issue | 🟠 MajorInteger division truncates audio trim/delay values.
track.startFrame / fps,track.endFrame / fps, andtrack.delayFrame / fpsare all integer divisions, silently truncating fractional seconds. For example, frame 15 at 30 fps yields0instead of0.5. TheframeToSecondshelper on line 202 already handles this correctly withtoDouble()but is unused here.Proposed fix
- val startSec = track.startFrame / fps - val endSec = track.endFrame / fps - val delayMs = (track.delayFrame / fps * 1000) + val startSec = track.startFrame.toDouble() / fps + val endSec = track.endFrame.toDouble() / fps + val delayMs = (track.delayFrame.toDouble() / fps * 1000).toLong()modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt (1)
102-112:⚠️ Potential issue | 🔴 Critical
field.first()crashes withNoSuchElementExceptionwhenselectedLyricsis empty.The backing field is initialized to
emptyList(). Any read ofselectedLyricsbefore it is populated (or if set to an empty list) will throw. This is reachable fromAppNavHost.onFinalize→LyricsActivity.start(... lyrics = viewModel.selectedLyrics ...).🐛 Proposed fix — guard against empty list
var selectedLyrics: List<SyncedLyricFrame> = emptyList() get() { + if (field.isEmpty()) return emptyList() val firstFrame = field.first().frame return field .map { SyncedLyricFrame( frame = it.frame - firstFrame, text = it.text, ) }.sortedBy { it.frame } }
🤖 Fix all issues with AI agents
In `@gradle/libs.versions.toml`:
- Around line 80-82: The nitrite and nitrite-mvstore entries are missing
explicit version pins which makes consumers brittle; update the
libs.versions.toml so both the nitrite and nitrite-mvstore declarations include
version.ref = "nitrite" (matching the existing nitrite-bom version key) so that
libs.bundles.nitrite works without requiring consumers to import the BOM
separately; locate the entries named nitrite and nitrite-mvstore and add
version.ref = "nitrite" to each.
In
`@modules/app/src/main/java/com/tejpratapsingh/animator/worker/SampleMotionWorker.kt`:
- Around line 71-77: The temp output file in SampleMotionWorker.getOutputFile()
is created in applicationContext.cacheDir which can be evicted before the user
taps the persistent notification; change creation to use a durable location
(e.g., applicationContext.filesDir) or, if you must use cacheDir, copy/move the
final video to filesDir (or the same durable location used by
MotionVideoContainer) inside SampleMotionWorker.onCompleted() before posting the
notification so the notification actions reference a stable file path; update
getOutputFile() and/or implement the copy-to-filesDir step in onCompleted() and
ensure any references to the file (notification intents) point to the durable
file.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.kt`:
- Around line 76-79: getOutputFile() currently returns a directory from
createProjectFolder() but must return an actual file path (e.g., create a .mp4
file with File.createTempFile() or append a filename to the directory) so
produceVideo() and encoders (jcodec/ffmpeg) receive a real file, not a folder;
also stop relying on volatile ProjectStore.motionProject in
provideCurrentProject() by persisting the project ID via inputData
(setCurrentProject() should store the ID into the WorkRequest input) and have
provideCurrentProject() load the project by that ID from inputData when present
so the worker can reconstruct the correct project after a process restart
(follow the SampleMotionWorker pattern for file creation and inputData usage).
In
`@modules/metadata-extractor/src/main/java/com/tejpratapsingh/motion/metadataextractor/presentation/ShareReceiverActivity.kt`:
- Around line 71-79: handleSharedText currently skips when no URL is found
leaving the loader visible and logs using a hardcoded "ShareReceiver" tag;
update handleSharedText to check if links.firstOrNull() is null and in that
branch hide the loading indicator (or call finish()), and show a user-facing
message (Toast or Snackbar) before finishing to avoid infinite spinner, and
replace the hardcoded Log.d tag with the companion object's TAG constant;
reference handleSharedText, extractLinks, metadataViewModel.getMetaData and TAG
when making changes.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/MotionProject.kt`:
- Around line 36-46: The provideCurrentProject() function has a TOCTOU race:
avoid generating Uuid.random() on every call and ensure
ProjectStore.motionProject is created only once by using a synchronized block
and double-checking the nullable field; specifically, check
ProjectStore.motionProject first, and if null enter a
synchronized(ProjectStore::class) (or a dedicated lock object) then recheck
motionProject, generate id and construct MotionProject only inside that
synchronized block and assign it to ProjectStore.motionProject before returning,
so concurrent callers cannot create different instances.
- Around line 20-23: The createOrSaveProject implementation discards the result
of getById and always calls insert, which causes a UniqueConstraintException for
existing MotionProject ids; replace the unconditional insert with Nitrite's
upsert by calling update(project, true) on the repository returned by
db.getRepository(MotionProject::class.java) inside
DatabaseProvider.createOrSaveProject so the MotionProject is updated when
present and inserted when missing.
🧹 Nitpick comments (18)
modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/worker/MotionWorker.kt (1)
115-116: Add KDoc for consistency with the other abstract methods.Every other abstract method in this class (
getMotionVideo,onProgress,onCompleted) has a KDoc block describing its purpose and parameters. Consider adding one here as well — e.g., clarifying that it's called onDispatchers.IOand that implementations may return either a temp file or a persistent location.📝 Suggested KDoc
+ /** + * Provides the output [File] where the generated video will be written. + * Called on [Dispatchers.IO]. Implementations may return a temporary or + * a persistent file location. + */ abstract fun getOutputFile(): Filemodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/adapter/AndroidVideoProducerAdapter.kt (1)
45-46: Implicit global dependency reduces testability.Fetching
motionConfigfrom a global singleton inside the function makes it harder to unit-test with different configurations. The previous approach of passing it as a parameter was more explicit and testable. Consider whether this trade-off is intentional.Also note the TOCTOU race in
provideCurrentConfig():MotionConfigStore.motionConfigcould becomenullbetween the!= nullcheck and the!!dereference if set from another thread.modules/metadata-extractor/src/main/java/com/tejpratapsingh/motion/metadataextractor/presentation/ShareReceiverActivity.kt (1)
82-89: Trailing\bin regex may truncate URLs ending with non-word characters.The word boundary
\bat the end of the pattern causes the regex engine to backtrack when a URL ends with characters like/,), or.— trimming them. For example,https://example.com/path/would be matched ashttps://example.com/path. Consider using Android'sPatterns.WEB_URLmatcher instead, or dropping the trailing\b:- val urlRegex = Regex("""\b(?:https?|ftp)://\S+\b""") + val urlRegex = Regex("""\b(?:https?|ftp)://\S+""")Note: without the trailing boundary you may want to trim common trailing punctuation (
.,,,)) in a post-processing step if the URL appears mid-sentence.modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/VideoAspectRatioParser.kt (2)
46-63: Avoid allocatingGson/GsonBuilderon every call.Both extension functions create a new Gson instance per invocation. Since
Gsonis thread-safe and immutable after creation, hoist them into top-levelvals to avoid repeated allocation and configuration overhead.♻️ Proposed refactor — cache Gson instances
+private val serializationGson = Gson() + +private val deserializationGson = + GsonBuilder() + .registerTypeAdapter( + VideoAspectRatio::class.java, + JsonDeserializer { element, _, _ -> + val obj = element.asJsonObject + val width = obj.get("width")?.asInt + ?: throw IllegalArgumentException("Missing 'width' in VideoAspectRatio JSON") + val height = obj.get("height")?.asInt + ?: throw IllegalArgumentException("Missing 'height' in VideoAspectRatio JSON") + val label = obj.get("label")?.asString ?: "${width}x${height}" + + allRatios.find { it.width == width && it.height == height } + ?: Custom(width, height, label) + }, + ).create() + -fun VideoAspectRatio.toJsonObject(): JsonObject = Gson().toJsonTree(this).asJsonObject +fun VideoAspectRatio.toJsonObject(): JsonObject = serializationGson.toJsonTree(this).asJsonObject -fun JsonObject.toVideoAspectRatio(): VideoAspectRatio { - val gson = - GsonBuilder() - .registerTypeAdapter( - VideoAspectRatio::class.java, - JsonDeserializer { element, _, _ -> - val obj = element.asJsonObject - val width = obj.get("width").asInt - val height = obj.get("height").asInt - val label = obj.get("label").asString - - allRatios.find { it.width == width && it.height == height } - ?: Custom(width, height, label) - }, - ).create() - return gson.fromJson(this, VideoAspectRatio::class.java) -} +fun JsonObject.toVideoAspectRatio(): VideoAspectRatio = + deserializationGson.fromJson(this, VideoAspectRatio::class.java)
26-44: Consider programmatically deriving the predefined ratios to reduce maintenance burden.The
allRatioslist is currently complete and accurate (all 16 sealed subclasses ofVideoAspectRatioare included). However, this hand-maintained list is fragile: if new predefined ratios are added toVideoAspectRatiowithout updatingallRatios, the deserializer will silently fall back toCustominstead of matching the canonical instance.Consider deriving this list programmatically—for example, using
VideoAspectRatio::class.nestedClasseswith reflection or a generated constant—to eliminate the manual synchronization requirement.gradle.properties (2)
23-31: These appear to be AGP compatibility flags opting out of stricter defaults.This is a reasonable migration strategy, but consider adding a comment (e.g.,
# TODO: migrate to new defaults) so these don't become permanent technical debt. Several of these flags (android.nonFinalResIds=false,android.uniquePackageNames=false,android.r8.strictFullModeForKeepRules=false) disable improvements that should eventually be adopted.
19-19:android.enableJetifier=trueis deprecated.Jetifier has been deprecated since AGP 7.x. If all your dependencies have migrated to AndroidX (which is likely given
compileSdk 36), you can safely remove this flag to reduce build overhead.modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/BaseFrameMotionView.kt (1)
81-82: CacheprovideCurrentConfig()in a local variable to avoid redundant calls.
provideCurrentConfig()is called twice inonMeasure, which runs frequently during layout. A single call stored in a local also guards against a (theoretical) config change between the two reads.♻️ Proposed fix
- val desiredWidth = provideCurrentConfig().aspectRatio.width - val desiredHeight = provideCurrentConfig().aspectRatio.height + val aspectRatio = provideCurrentConfig().aspectRatio + val desiredWidth = aspectRatio.width + val desiredHeight = aspectRatio.heightmodules/app/src/main/java/com/tejpratapsingh/animator/presentation/SampleMotionVideo.kt (1)
24-24: Global mutable config is set before constructing the producer — be aware of thread-safety implications.
setCurrentConfigwrites to a shared mutable singleton. If multiple callers (e.g., different workers or activities) invokesampleMotionVideoconcurrently with different configs, they will race on the global state. Consider documenting that this must only be called from a single-threaded entry point, or moving to a scoped config approach in the future.modules/app/src/main/java/com/tejpratapsingh/animator/ui/view/RenaultCar.kt (1)
51-60: Minor: two calls toprovideCurrentConfig()can be consolidated.Same pattern as in
BaseFrameMotionView.onMeasure. While these lambdas likely execute once, caching in a local is cleaner and consistent.♻️ Proposed fix
+ val currentAspectRatio = provideCurrentConfig().aspectRatio contourHeightOf { - provideCurrentConfig() - .aspectRatio.height + currentAspectRatio.height .toYInt() } contourWidthOf { - provideCurrentConfig() - .aspectRatio.width + currentAspectRatio.width .toXInt() }modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/extensions/ContextExtensions.kt (1)
19-22:exists()check beforemkdirs()is redundant.
mkdirs()already returnsfalsewithout error if the directory exists. The explicitexists()guard can be removed for both directory creations.♻️ Proposed fix
fun Context.createProjectFolder(motionProject: MotionProject): File { val projectsDirectory: File = applicationContext.filesDir.resolve(PROJECTS_DIR) - if (!projectsDirectory.exists()) { - projectsDirectory.mkdirs() - } - val fileDirectory: File = projectsDirectory.resolve(motionProject.id) - if (!fileDirectory.exists()) { - fileDirectory.mkdirs() - } + val fileDirectory: File = projectsDirectory.resolve(motionProject.id) + fileDirectory.mkdirs() return fileDirectory }modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/MotionVideoProducer.kt (1)
28-43: Removingconfigfromwith()makes configuration implicit — document the contract.Callers must now call
setCurrentConfig()beforeMotionVideoProducer.with(). This implicit ordering dependency is easy to miss and could lead to silent use of default config values. Consider adding a KDoc comment on thewith()method noting this prerequisite, or adding a runtime check (e.g., logging a warning if no config has been set).modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionConfig.kt (1)
24-29:provideCurrentConfig()performs two separate volatile reads — use a local variable.Lines 25 and 26 each perform an independent read of
MotionConfigStore.motionConfig. While not currently buggy (the value never reverts tonull), this is fragile and the!!is an unnecessary forced unwrap. A single read into a local eliminates the concern.♻️ Proposed fix
-fun provideCurrentConfig(): MotionConfig = - if (MotionConfigStore.motionConfig != null) { - MotionConfigStore.motionConfig!! - } else { - MotionConfig() - } +fun provideCurrentConfig(): MotionConfig = + MotionConfigStore.motionConfig ?: MotionConfig()modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/lrclib/model/LyricsResponse.kt (1)
10-29: DuplicateLyricsResponseclass in a different package — potential confusion.There is already a
LyricsResponseincom.tejpratapsingh.lyricsmaker.data.api.model(see relevant snippet atmodules/lyrics-maker/.../data/api/model/LyricsResponse.kt, lines 8–45) with a different field set (duration: Float?, nonamefield, defaults onalbumName/instrumental). Having two classes with the same name in sibling packages will cause import confusion and maintenance burden. Consider consolidating or clearly differentiating them (e.g., renaming this one toLrcLibLyricsResponse).modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/LyricsVideoProducer.kt (1)
27-28: Global mutable config is not thread-safe and couples callers to ordering.
setCurrentConfigwrites to a shared singleton (MotionConfigStore.motionConfig). IfgetLyricsVideoProducerandgetMultiLyricsVideoProducer(or any other producer factory) are ever invoked concurrently or interleaved, the last writer wins and the other caller silently picks up the wrong config. The previous design of passingconfigas a parameter toMotionVideoProducer.with(...)was safer.If the global-config pattern is intentional for this PR, at a minimum document or enforce that callers must be serialized (e.g., single-threaded dispatcher). Preferably, restore parameter-based config injection so each producer is self-contained.
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt (1)
83-89: Debug toast"Success ${it.lyrics.size}"appears to be a leftover — remove or replace with meaningful UX.This toast exposes internal result counts to the user. If it's a debug artifact, remove it; if user feedback is needed on success, consider navigating directly or showing a more meaningful message.
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt (2)
50-52: ExposingMutableStateFlowdirectly breaks unidirectional data flow.
query,socialMeta, andselectedLyricare all exposed asMutableStateFlow, allowing any consumer to mutate ViewModel state directly. Consider exposing read-onlyStateFlowand providing dedicated mutation functions (likesearchLyricsalready does for_uiState).♻️ Sketch
- val query: MutableStateFlow<String> = MutableStateFlow("") - val socialMeta: MutableStateFlow<SocialMeta> = MutableStateFlow(SocialMeta()) - val selectedLyric: MutableStateFlow<LyricsResponse?> = MutableStateFlow(null) + private val _query = MutableStateFlow("") + val query: StateFlow<String> = _query.asStateFlow() + + private val _socialMeta = MutableStateFlow(SocialMeta()) + val socialMeta: StateFlow<SocialMeta> = _socialMeta.asStateFlow() + + private val _selectedLyric = MutableStateFlow<LyricsResponse?>(null) + val selectedLyric: StateFlow<LyricsResponse?> = _selectedLyric.asStateFlow() + + fun updateQuery(value: String) { _query.value = value } + fun updateSocialMeta(value: SocialMeta) { _socialMeta.value = value } + fun selectLyric(value: LyricsResponse?) { _selectedLyric.value = value }
42-46: Manual wiring of OkHttp/Gson/ApiService/Repository inside the ViewModel.Constructing these collaborators directly makes the ViewModel hard to test (no way to inject mocks). Consider using a
ViewModelFactory, Hilt, or constructor injection to supply the repository.
modules/app/src/main/java/com/tejpratapsingh/animator/worker/SampleMotionWorker.kt
Outdated
Show resolved
Hide resolved
...maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.kt
Outdated
Show resolved
Hide resolved
.../main/java/com/tejpratapsingh/motion/metadataextractor/presentation/ShareReceiverActivity.kt
Show resolved
Hide resolved
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/MotionProject.kt
Outdated
Show resolved
Hide resolved
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/MotionProject.kt
Outdated
Show resolved
Hide resolved
tejpratap46
force-pushed
the
feat/implement-sdui
branch
from
3021260 to
be8187c
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
modules/tensorflow-motion-ext/build.gradle (1)
48-54:⚠️ Potential issue | 🔴 CriticalThe Kotlin plugin removal breaks compilation—this module contains 7 Kotlin source files.
Removing the Kotlin Android plugin will prevent the build from compiling the Kotlin files in this module:
ImageUtils.kt,TensorFlowImageProcessor.kt,CarBgRemover.kt,TfLiteSegmentationHelper.kt,TiledBackgroundRemover.kt,SuperResolutionProcessor.kt, andExampleUnitTest.kt. The Kotlin plugin must be retained to compile these sources.modules/metadata-extractor/src/main/java/com/tejpratapsingh/motion/metadataextractor/presentation/ShareReceiverActivity.kt (2)
54-68:⚠️ Potential issue | 🟠 MajorActivity stuck on loading spinner when
intent.actionis notACTION_SEND.The outer
whenonintent?.actionhas noelsebranch. If the activity is launched with an unexpected or null action, neither metadata fetch norfinish()is called, leaving the user stuck on the loading indicator.Proposed fix
private fun findSharedUrl() { when (intent?.action) { Intent.ACTION_SEND -> { when (intent.type) { "text/plain" -> { handleSharedText(intent) } else -> { // Handle other types if needed finish() // Close the activity if the type is unsupported } } } + + else -> { + finish() + } } }
119-124:⚠️ Potential issue | 🟡 MinorActivity left in a dead state after metadata fetch error.
On error, the loading spinner is hidden and a toast is shown, but the activity remains open with no actionable UI. Consider calling
finish()after the toast (or providing a retry option) so the user isn't stranded.Proposed fix (finish after error)
is MetaDataResult.Error -> { Log.e(TAG, "Received error", result.error) Toast .makeText(this, "Failed to fetch metadata", Toast.LENGTH_SHORT) .show() + finish() }modules/pytorch-motion-ext/build.gradle (1)
1-4:⚠️ Potential issue | 🔴 CriticalRestore the
kotlin-androidplugin — this module contains multiple Kotlin source files that require it to compile.The
kotlin-androidplugin was removed from the plugins block, but the module contains 9 Kotlin source files (.kt):
- PyTorchImageProcessor.kt
- ModelTypes.kt
- RemoveBg.kt
- Remover.kt
- ImageUpscaler.kt
- SuperRes.kt
- FileUtils.kt
- NetUtils.kt
- ExampleUnitTest.kt
Without the Kotlin plugin, these files will fail to compile. The plugin must be restored.
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt (1)
79-100:⚠️ Potential issue | 🟠 Major
runBlockingin a View'sinitblocks the main thread for network I/O.This performs synchronous network calls (MusicBrainz lookup + image download) on the UI thread during view construction. This will cause ANRs or significant jank. Consider launching a coroutine and updating the
ImageViewasynchronously.🐛 Sketch of an async approach
- ivAlbumArt.apply { - runBlocking { - if (image != null) { + // Launch async instead of blocking UI thread + kotlinx.coroutines.CoroutineScope(Dispatchers.Main).launch { + if (image != null) { ... - } else { + } else { ... - } } - } + bitmap?.let { ivAlbumArt.setImageBitmap(it) } + }A proper lifecycle-aware scope (e.g., from the hosting Activity/Fragment) would be even better to avoid leaks.
modules/ffmpeg-motion-ext/src/main/java/com/tejpratapsingh/motionlib/ffmpeg/FfmpegVideoProducerAdapter.kt (1)
148-151:⚠️ Potential issue | 🟠 MajorPre-existing: integer division silently truncates audio timing values.
Lines 149-151 use integer division (
track.startFrame / fps) which truncates fractional seconds. For example, frame 15 at 30fps yields0instead of0.5. TheframeToSecondshelper on line 202 handles this correctly but isn't used here.Not introduced by this PR, but worth fixing since incorrect audio sync could be hard to debug.
Suggested fix
- val startSec = track.startFrame / fps - val endSec = track.endFrame / fps - val delayMs = (track.delayFrame / fps * 1000) + val startSec = frameToSeconds(track.startFrame, fps) + val endSec = frameToSeconds(track.endFrame, fps) + val delayMs = (frameToSeconds(track.delayFrame, fps) * 1000).toLong()
🤖 Fix all issues with AI agents
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/lrclib/client/LrcLibApiServiceImpl.kt`:
- Around line 68-70: In LrcLibApiServiceImpl's catch blocks (the one wrapping
the network call and the one in the get method), do not swallow
CancellationException: detect and rethrow CancellationException before
converting other errors into Result.failure; e.g., replace the single catch (e:
Exception) with logic that if e is CancellationException then throw e, otherwise
return Result.failure(e), ensuring structured concurrency and proper coroutine
cancellation propagation.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt`:
- Around line 83-90: The Success toast in SearchActivity is a debug
placeholder—remove the Toast.makeText(...) inside the LyricsUiState.Success
branch or replace its message with a user-facing string (e.g., "Lyrics loaded"
or localized resource) and ensure any UI feedback is meaningful; also simplify
the when handling by collapsing empty Initial and Loading branches into an else
or handling only Success and Error cases so the when is concise and not left
with no-op branches.
In `@modules/motion-store/build.gradle`:
- Around line 8-10: The compileSdk block uses invalid AGP DSL syntax (compileSdk
{ version = release(36) }); replace it with the standard form used in other
modules — set compileSdk directly (e.g., compileSdk = 36) instead of using
release(36) block, ensuring the compileSdk declaration in the module (the
compileSdk symbol) matches the rest of the project.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/DatabaseProvider.kt`:
- Around line 1-4: This file is missing the Kotlin package declaration which
prevents compilation; add the package declaration "package
com.tejpratapsingh.motionstore" as the very first line of the
DatabaseProvider.kt file (before the import statements) so the DatabaseProvider
top-level declarations/types resolve correctly.
- Around line 9-19: The MVStoreModule instance (storeModule) is configured but
never loaded into the Nitrite builder so the DB stays in-memory; update the
Nitrite builder used to create db (the Nitrite.builder() call that currently
only loads JacksonMapperModule) to also load the configured storeModule via
loadModule(storeModule) so the database uses the on-disk MVStore backing instead
of an in-memory store.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/extensions/ContextExtensions.kt`:
- Around line 18-34: The function Context.createProjectFolder is misnamed and
its KDoc is inaccurate because it returns a temporary .mp4 File, not a
directory; rename the function to createProjectOutputFile (or similar), update
the KDoc to state it creates and returns a temp output .mp4 file inside the
project's internal storage directory, and update the inline comment that
currently says "// Use cacheDir for temp files that can be cleared" to
accurately reflect that fileDirectory (internal filesDir) is used (or switch to
cacheDir if you intend truly ephemeral temp files); ensure all call sites and
imports referencing createProjectFolder are updated to the new name.
🧹 Nitpick comments (13)
modules/lyrics-maker/src/main/res/layout/lyrics_container.xml (1)
70-84: Stale constraint on hiddenpb_progress.
pb_progressis nowvisibility="gone"but still carrieslayout_constraintBottom_toTopOf="@+id/fake_chart_view"(line 82). Sincefake_chart_view's bottom anchor changed frombtn_containertoparent, this reference is now semantically stale. It's harmless while the view isgone, but if it's ever made visible again, the constraint chain may not behave as expected. Consider updating or removing it for clarity.modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/MotionVideoProducer.kt (1)
68-69: Misleading comment: saysDispatchers.Defaultbut code usesDispatchers.IO.The comment on line 69 says "Use Dispatchers.Default for CPU-bound work" while the coroutine actually runs on
Dispatchers.IO. This is pre-existing but now more visible. Consider updating the comment to match the actual dispatcher, or switching toDispatchers.Defaultif the work is truly CPU-bound (video frame rendering).✏️ Suggested comment fix
withContext(Dispatchers.IO) { - // Use Dispatchers.Default for CPU-bound work + // Use Dispatchers.IO for file I/O and video productionmodules/metadata-extractor/src/main/java/com/tejpratapsingh/motion/metadataextractor/presentation/ShareReceiverActivity.kt (1)
82-89: Consider usingandroid.util.Patterns.WEB_URLfor more robust URL matching.The hand-rolled regex works for common cases but may over-capture trailing punctuation (e.g.,
),.at end of a sentence) or miss edge cases. Android provides a well-tested pattern for this purpose:private fun extractLinks(text: String): List<String> { val matcher = android.util.Patterns.WEB_URL.matcher(text) return buildList { while (matcher.find()) { add(matcher.group()) } } }modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/VideoAspectRatioParser.kt (2)
46-63: Consider caching theGsoninstances instead of recreating them on every call.Both
toJsonObject()(line 46) andtoVideoAspectRatio()(lines 49-62) allocate a newGson/GsonBuilderon every invocation. If these are called in a loop (e.g., serializing a list of ratios), this adds unnecessary GC pressure. Extract the configuredGsoninstance to acompanion objector a top-levelprivate val.♻️ Suggested extraction
+private val aspectRatioGson: Gson = + GsonBuilder() + .registerTypeAdapter( + VideoAspectRatio::class.java, + JsonDeserializer { element, _, _ -> + val obj = element.asJsonObject + val width = obj.get("width")?.asInt + ?: throw IllegalArgumentException("Missing 'width' in VideoAspectRatio JSON") + val height = obj.get("height")?.asInt + ?: throw IllegalArgumentException("Missing 'height' in VideoAspectRatio JSON") + val label = obj.get("label")?.asString ?: "${width}x${height}" + + allRatios.find { it.width == width && it.height == height } + ?: Custom(width, height, label) + }, + ).create() + -fun VideoAspectRatio.toJsonObject(): JsonObject = Gson().toJsonTree(this).asJsonObject +fun VideoAspectRatio.toJsonObject(): JsonObject = aspectRatioGson.toJsonTree(this).asJsonObject -fun JsonObject.toVideoAspectRatio(): VideoAspectRatio { - val gson = - GsonBuilder() - .registerTypeAdapter( - VideoAspectRatio::class.java, - JsonDeserializer { element, _, _ -> - val obj = element.asJsonObject - val width = obj.get("width").asInt - val height = obj.get("height").asInt - val label = obj.get("label").asString - - allRatios.find { it.width == width && it.height == height } - ?: Custom(width, height, label) - }, - ).create() - return gson.fromJson(this, VideoAspectRatio::class.java) -} +fun JsonObject.toVideoAspectRatio(): VideoAspectRatio = + aspectRatioGson.fromJson(this, VideoAspectRatio::class.java)
26-44: Consider using sealed subclass introspection to keepallRatiosin sync withVideoAspectRatiodefinitions.If a new predefined ratio is added to
VideoAspectRatio, this hand-maintained list will silently miss it, causing the deserializer to fall back toCustominstead of resolving the known instance. SinceVideoAspectRatiois a sealed class, you could useVideoAspectRatio::class.sealedSubclasses.filter { it.isData && it.objectInstance != null }.mapNotNull { it.objectInstance as? VideoAspectRatio }to build this list automatically. Alternatively, consider adding a comment near this list documenting that new predefined ratios must be added here.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/LyricsActivity.kt (1)
58-66: Global side effect inside alazyproperty initializer is fragile.
setCurrentProject(...)mutates global state (ProjectStore.motionProject) as a side effect of lazily initializingvideo. This creates an implicit ordering dependency — any code that reads the current project beforevideois accessed will see stale/null state, and the intent to set the project is hidden inside a property getter. Consider movingsetCurrentProject(...)toonCreate()to make the lifecycle and ordering explicit.♻️ Proposed refactor
+ override fun onCreate(savedInstanceState: Bundle?) { + super.onCreate(savedInstanceState) + setCurrentProject( + MotionProject( + id = "lyrics", + name = song, + path = "/lyrics", + sdui = null, + ), + ) + + val start = lyrics.minBy { it.frame }.frameprivate val video by lazy { - setCurrentProject( - MotionProject( - id = "lyrics", - name = song, - path = "/lyrics", - sdui = null, - ), - ) getLyricsVideoProducer(modules/3d-filament-renderer/src/main/java/com/tejpratapsingh/motionlib/filamentrenderer/Filament3dView.kt (1)
69-86: Consider caching the config to avoid inconsistent reads.
provideCurrentConfig()is called 4 separate times during initialization. If the global config were mutated between calls, the surface buffer size and viewport could disagree. Caching it in a local variable would be safer and cleaner.♻️ Suggested refactor
private fun initializeFilament() { + val config = provideCurrentConfig() Utils.init() surfaceTexture = SurfaceTexture(0) surfaceTexture.setDefaultBufferSize( - provideCurrentConfig().aspectRatio.width, - provideCurrentConfig().aspectRatio.height, + config.aspectRatio.width, + config.aspectRatio.height, ) surface = Surface(surfaceTexture) engine = Engine.create() swapChain = engine.createSwapChain(surface, 0) scene = engine.createScene() view = engine.createView() view.scene = scene renderer = engine.createRenderer() view.viewport = Viewport( 0, 0, - provideCurrentConfig().aspectRatio.width, - provideCurrentConfig().aspectRatio.height, + config.aspectRatio.width, + config.aspectRatio.height, ) }modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SearchLyricsCompose.kt (1)
93-146: Preferitems(list)overitems(list.size)with index access.Using
items(state.lyrics)with a lambda parameter is more idiomatic Compose and avoids repeated index-based access. You can also provide akeyfor stable recomposition.♻️ Suggested refactor
- items(state.lyrics.size) { item -> - Card( - modifier = - Modifier - .fillMaxWidth() - .padding(vertical = 4.dp) - .clickable { onLyricsSelected(state.lyrics[item]) }, + items( + count = state.lyrics.size, + key = { state.lyrics[it].trackName + state.lyrics[it].artistName }, + ) { item -> + val lyrics = state.lyrics[item] + Card( + modifier = + Modifier + .fillMaxWidth() + .padding(vertical = 4.dp) + .clickable { onLyricsSelected(lyrics) },modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionConfig.kt (1)
24-29:provideCurrentConfig()has a double-read on a volatile field — use a local variable.Between the
!= nullcheck (line 25) and the!!dereference (line 26), another thread could theoretically interleave. While nothing currently nullifies the store, this is a fragile pattern. Use a localvalto read once.♻️ Proposed fix
fun provideCurrentConfig(): MotionConfig = - if (MotionConfigStore.motionConfig != null) { - MotionConfigStore.motionConfig!! - } else { - MotionConfig() - } + MotionConfigStore.motionConfig ?: MotionConfig()modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/BaseFrameMotionView.kt (1)
81-82: Cache the config to avoid redundant calls.
provideCurrentConfig()is called twice in the sameonMeasurepass. If the store is null, each call allocates a new defaultMotionConfig(). Capture it in a local val for consistency and to avoid the extra allocation, especially sinceonMeasurecan be called frequently.Suggested fix
- val desiredWidth = provideCurrentConfig().aspectRatio.width - val desiredHeight = provideCurrentConfig().aspectRatio.height + val config = provideCurrentConfig() + val desiredWidth = config.aspectRatio.width + val desiredHeight = config.aspectRatio.heightmodules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/ConfigParser.kt (1)
7-15: Consider reusing a singleGsoninstance.A new
Gson()is allocated on every serialization call. If this is called frequently, extract a companion-level or module-level instance.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt (2)
42-46: Dependencies are hard-wired — consider constructor injection for testability.
OkHttpClient,Gson,LrcLibApiService, andLyricsRepositoryare all instantiated inline, making it impossible to substitute test doubles. If you plan to test this ViewModel, consider accepting the repository (at minimum) as a constructor parameter via aViewModelFactoryor a DI framework.
90-100:lyricsgetter re-parses on every access — consider caching or deriving reactively.Each property access re-calls
LrcHelper.getSyncedLyrics(...)andprovideCurrentConfig(). If this getter is called from a composable that recomposes frequently, it will redundantly parse lyrics each time. Consider deriving this as aStateFlow(e.g., viacombine/maponselectedLyric) so the parsing happens once per selection change.
| } catch (e: Exception) { | ||
| Result.failure(e) | ||
| } |
There was a problem hiding this comment.
catch (e: Exception) swallows CancellationException, breaking structured concurrency.
When a coroutine is cancelled (e.g., ViewModel is cleared), CancellationException must propagate for proper cleanup. Catching all Exception and wrapping it in Result.failure silently prevents cancellation. Re-throw CancellationException before handling other errors. The same issue applies to the get method at line 116-118.
Proposed fix (apply to both methods)
+import kotlin.coroutines.cancellation.CancellationException } catch (e: Exception) {
+ if (e is CancellationException) throw e
Result.failure(e)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (e: Exception) { | |
| Result.failure(e) | |
| } | |
| } catch (e: Exception) { | |
| if (e is CancellationException) throw e | |
| Result.failure(e) | |
| } |
🤖 Prompt for AI Agents
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/lrclib/client/LrcLibApiServiceImpl.kt`
around lines 68 - 70, In LrcLibApiServiceImpl's catch blocks (the one wrapping
the network call and the one in the get method), do not swallow
CancellationException: detect and rethrow CancellationException before
converting other errors into Result.failure; e.g., replace the single catch (e:
Exception) with logic that if e is CancellationException then throw e, otherwise
return Result.failure(e), ensuring structured concurrency and proper coroutine
cancellation propagation.
...s-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt
Outdated
Show resolved
Hide resolved
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/DatabaseProvider.kt
Outdated
Show resolved
Hide resolved
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/DatabaseProvider.kt
Outdated
Show resolved
Hide resolved
...es/motion-store/src/main/java/com/tejpratapsingh/motionstore/extensions/ContextExtensions.kt
Outdated
Show resolved
Hide resolved
tejpratap46
force-pushed
the
feat/implement-sdui
branch
5 times, most recently
from
9a65385 to
b7ea281
Compare
tejpratap46
force-pushed
the
feat/implement-sdui
branch
from
b7ea281 to
b4a4477
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| .toJsonTree( | ||
| mapOf( | ||
| "startFrame" to startFrame, | ||
| "endFrame" to startFrame, |
There was a problem hiding this comment.
CRITICAL: Copy-paste bug — endFrame value is serialized as startFrame
The map key is "endFrame" but the value is startFrame. This means the serialized JSON will always have endFrame == startFrame, silently corrupting any round-trip deserialization of MotionEffect.
| "endFrame" to startFrame, | |
| "endFrame" to endFrame, |
| .toJsonTree( | ||
| mapOf( | ||
| "startFrame" to startFrame, | ||
| "endFrame" to startFrame, |
There was a problem hiding this comment.
CRITICAL: Copy-paste bug — endFrame value is serialized as startFrame
Same issue as MotionEffectParser: the "endFrame" key maps to startFrame instead of endFrame. Any MotionView serialized with this function will have endFrame == startFrame in the JSON, breaking SDUI playback duration.
| "endFrame" to startFrame, | |
| "endFrame" to endFrame, |
Code Review SummaryStatus: 2 New Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Other Observations (not in diff)Issues found in changed code that warrant attention but are outside inline-commentable diff lines:
Files Reviewed (28 files)
|
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
modules/metadata-extractor/src/main/java/com/tejpratapsingh/motion/metadataextractor/presentation/ShareReceiverActivity.kt (2)
96-103:⚠️ Potential issue | 🟡 MinorHardcoded
"ShareReceiver"log tag persists inobserverMetaData.Lines 98 and 102 still use the literal
"ShareReceiver"instead of theTAGconstant — the same class of issue that was fixed inhandleSharedText.🐛 Proposed fix
- Log.d("ShareReceiver", "Received result: $result") + Log.d(TAG, "Received result: $result") ... - Log.d("ShareReceiver", "Received metadata: ${result.metaData}") + Log.d(TAG, "Received metadata: ${result.metaData}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/metadata-extractor/src/main/java/com/tejpratapsingh/motion/metadataextractor/presentation/ShareReceiverActivity.kt` around lines 96 - 103, The observerMetaData() function currently uses the hardcoded log tag "ShareReceiver" in the metadataViewModel.metadata.observe lambda (two Log.d calls); change those Log.d invocations to use the class TAG constant instead (i.e., replace "ShareReceiver" with TAG) so it matches the handleSharedText fix and ensure the TAG constant (e.g., companion object TAG) is available in the same class.
110-121:⚠️ Potential issue | 🟠 MajorUnguarded implicit
startActivitywill crash if no activity handlesACTIVITY_INTENT_ACTION.Since
ShareReceiverActivitylives in a library module, the consuming app is responsible for registering an activity forACTIVITY_INTENT_ACTION. If it hasn't (or on Android 11+ where package visibility rules apply),startActivitythrowsActivityNotFoundException, crashing the app with no user-facing feedback.🛡️ Proposed fix
val onDone = { - startActivity( - Intent(ACTIVITY_INTENT_ACTION).apply { - putExtra( - EXTRA_METADATA, - result.metaData - .copy(title = binding.tvTitle.text.toString()), - ) - }, - ) - finish() + val intent = Intent(ACTIVITY_INTENT_ACTION).apply { + putExtra( + EXTRA_METADATA, + result.metaData.copy(title = binding.tvTitle.text.toString()), + ) + } + try { + startActivity(intent) + finish() + } catch (e: android.content.ActivityNotFoundException) { + Log.e(TAG, "No activity found for action: $ACTIVITY_INTENT_ACTION", e) + Toast.makeText(this, "Unable to continue: no handler found", Toast.LENGTH_SHORT).show() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/metadata-extractor/src/main/java/com/tejpratapsingh/motion/metadataextractor/presentation/ShareReceiverActivity.kt` around lines 110 - 121, The onDone lambda in ShareReceiverActivity calls startActivity with an implicit Intent using ACTIVITY_INTENT_ACTION without guarding for no receiver; this can throw ActivityNotFoundException in library consumers. Fix by resolving/checking the Intent before calling startActivity (use intent.resolveActivity(packageManager) or packageManager.queryIntentActivities) and if no handler is found, handle gracefully (e.g., show a user-facing message via Toast/Snackbar or log and abort) or wrap startActivity in a try/catch for ActivityNotFoundException; update the onDone closure around startActivity/Intent(EXTRA_METADATA) accordingly so the app never crashes when no activity handles ACTIVITY_INTENT_ACTION.modules/lyrics-maker/src/main/res/layout/lyrics_container.xml (1)
114-143:⚠️ Potential issue | 🟡 Minor
ImageButtons insidebtn_containerare missingandroid:contentDescription.
btn_previous,btn_play_pause, andbtn_nextall useandroid:srconly, with nocontentDescription. While the container isgonenow, the SDUI context of this PR suggests these controls may be surfaced programmatically at runtime. When/if shown, TalkBack will announce them as "unlabelled" or read the resource filename.♿ Proposed fix — add contentDescription to each ImageButton
<ImageButton android:id="@+id/btn_previous" ... + android:contentDescription="@string/btn_previous_description" android:src="@android:drawable/ic_media_previous" /> <ImageButton android:id="@+id/btn_play_pause" ... + android:contentDescription="@string/btn_play_pause_description" android:src="@android:drawable/ic_media_pause" /> <ImageButton android:id="@+id/btn_next" ... + android:contentDescription="@string/btn_next_description" android:src="@android:drawable/ic_media_next" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/res/layout/lyrics_container.xml` around lines 114 - 143, Add accessible labels by setting android:contentDescription on the ImageButtons with ids btn_previous, btn_play_pause, and btn_next; update the layout for each ImageButton to include a clear, localized description (e.g., "Previous", "Play/Pause", "Next" or string resource references like `@string/btn_previous_desc`, `@string/btn_play_pause_desc`, `@string/btn_next_desc`) so TalkBack will announce them correctly when the container is made visible.modules/pytorch-motion-ext/build.gradle (1)
1-4:⚠️ Potential issue | 🔴 Critical
kotlin.androidplugin must be restored — this module contains 9 Kotlin source files.The module contains Kotlin files that require the
kotlin.androidplugin to compile:
PyTorchImageProcessor.ktModelTypes.ktRemoveBg.ktRemover.ktImageUpscaler.ktSuperRes.ktFileUtils.ktNetUtils.ktExampleUnitTest.ktRemoving
alias(libs.plugins.kotlin.android)without a replacement will break the build.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/pytorch-motion-ext/build.gradle` around lines 1 - 4, The plugins block removed the Kotlin Android plugin but the module contains Kotlin sources (e.g. PyTorchImageProcessor.kt, ModelTypes.kt, RemoveBg.kt), so restore the Kotlin Android plugin by adding back the kotlin.android plugin entry (for example re-add alias(libs.plugins.kotlin.android) or id 'org.jetbrains.kotlin.android') into the plugins { ... } block alongside alias(libs.plugins.android.library) and id 'maven-publish' so the Kotlin sources will compile.modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/worker/MotionWorker.kt (1)
111-113:⚠️ Potential issue | 🟡 Minor
onFailedshould besuspendfor consistency withonProgressandonCompleted.
onProgressandonCompletedwere madeabstract suspend fun, butonFailedremains a plainopen fun. Subclasses that need to do async work on failure (e.g., dismiss a foreground notification, log to a remote service) are forced to launch a new coroutine themselves.♻️ Proposed fix
- open fun onFailed(exception: Exception) { + open suspend fun onFailed(exception: Exception) { // Default implementation does nothing, subclasses can override. }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/worker/MotionWorker.kt` around lines 111 - 113, The onFailed implementation in MotionWorker is non-suspending while onProgress and onCompleted are suspend functions; change the signature of open fun onFailed(exception: Exception) to open suspend fun onFailed(exception: Exception) so subclasses can perform coroutine work directly (update MotionWorker.onFailed declaration and ensure any subclass overrides are marked suspend or updated accordingly, e.g., methods overriding onFailed must now be suspend functions to match the new signature).modules/ivi-demo/src/main/java/com/tejpratapsingh/ivi_demo/motion/RenaultCar.kt (1)
1-1:⚠️ Potential issue | 🔴 CriticalAdd missing package declaration as the first line.
The file is missing its package declaration. It should start with
package com.tejpratapsingh.ivi_demo.motionbefore any imports. Without it, the class lands in the default package, breaking cross-module imports and violating Kotlin conventions. Add this as line 1, with all imports following it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/ivi-demo/src/main/java/com/tejpratapsingh/ivi_demo/motion/RenaultCar.kt` at line 1, This file is missing its package declaration; add the line "package com.tejpratapsingh.ivi_demo.motion" as the very first line of RenaultCar.kt so the RenaultCar class (and any top-level declarations in this file) are placed in the correct package and imports remain after that declaration.modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/infra/AndroidVideoGenerator.kt (1)
52-54:⚠️ Potential issue | 🟡 MinorRemove stale
@param motionConfigfrom KDoc.The
motionConfigparameter was removed fromgenerateVideo, but its KDoc entry was not cleaned up. IDEs and generated docs will show a dangling@paramthat refers to a non-existent parameter.🐛 Proposed fix
* `@param` outputFile The [File] object where the generated video will be saved. * If an error occurs during generation and this file exists, it will be deleted. - * `@param` motionConfig A [MotionConfig] object specifying video properties like - * width, height, and frames per second (fps). * `@param` motionAudios A list of [MotionAudio] objects to be mixed into the video.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/infra/AndroidVideoGenerator.kt` around lines 52 - 54, The KDoc for AndroidVideoGenerator.generateVideo still contains a stale `@param` motionConfig entry even though the motionConfig parameter was removed; remove the dangling `@param` line from the function's KDoc (the comment block above the generateVideo method) so the documentation matches the current signature and no longer references motionConfig.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SyncedLyricsSelector.kt (1)
304-304:⚠️ Potential issue | 🟠 MajorSide-effect callback called directly during composition — use
LaunchedEffect.
onSelectionChanged(selected)is invoked in the composition body on every recomposition, not only whenselectedchanges. If the callback triggers any external state mutation (e.g., updating aViewModel), it can cause an infinite recomposition cycle. Wrap it inLaunchedEffect(selected)so it is only dispatched when the selection actually changes.🐛 Proposed fix
- onSelectionChanged(selected) + LaunchedEffect(selected) { + onSelectionChanged(selected) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SyncedLyricsSelector.kt` at line 304, The call onSelectionChanged(selected) in the SyncedLyricsSelector composable is a side-effect executed during composition and should be moved into a LaunchedEffect keyed on selected; modify the SyncedLyricsSelector composable to remove the direct onSelectionChanged(selected) invocation from the composition body and instead call onSelectionChanged(selected) inside LaunchedEffect(selected) (or another appropriate side-effect API) so the callback runs only when selected actually changes and avoids recomposition loops.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt (1)
102-112:⚠️ Potential issue | 🔴 Critical
field.first()crashes withNoSuchElementExceptionwhenselectedLyricsis empty.The backing field is initialised to
emptyList()and any caller that readsselectedLyricsbefore assigning a non-empty list will hit an unhandled exception.🐛 Proposed fix
var selectedLyrics: List<SyncedLyricFrame> = emptyList() get() { + if (field.isEmpty()) return emptyList() val firstFrame = field.first().frame return field .map {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt` around lines 102 - 112, The custom getter for selectedLyrics uses field.first() which throws NoSuchElementException when the backing field is empty; update the getter in LyricsViewModel (selectedLyrics) to guard against an empty field (e.g., if (field.isEmpty()) return emptyList()) or use firstOrNull() and return emptyList() when null, otherwise compute the normalized frames into SyncedLyricFrame and sort; keep the rest of the mapping logic unchanged so callers receive an empty list instead of an exception.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/LyricsVideoProducer.kt (1)
29-37:⚠️ Potential issue | 🟠 Major
lyrics.first()/lyrics.last()throwNoSuchElementExceptionon an empty list.No guard exists before using
lyrics.first()andlyrics.last()to derivestartFrame/endFrame. If the caller passes an empty list the function crashes immediately.🐛 Proposed fix
+ require(lyrics.isNotEmpty()) { "lyrics must not be empty" } + val motionView = LyricsContainer(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/LyricsVideoProducer.kt` around lines 29 - 37, The code uses lyrics.first() and lyrics.last() to set startFrame/endFrame in the LyricsContainer construction which will throw NoSuchElementException for an empty lyrics list; update the calling code that creates LyricsContainer (in LyricsVideoProducer.kt) to guard against an empty lyrics list by computing safe start/end frames (e.g., use lyrics.firstOrNull()?.frame and lyrics.lastOrNull()?.frame or provide sensible default frames) and handle the empty-case (skip creating LyricsContainer, return early, or supply defaults) so LyricsContainer receives valid startFrame and endFrame values.modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/adapter/AndroidVideoProducerAdapter.kt (1)
62-65:⚠️ Potential issue | 🟡 MinorSilent error swallowing on frame save failure.
When saving a frame fails, the exception is logged but
produceVideoreturns the (incomplete/empty)outputFilewithout signaling the failure to the caller. The caller has no way to distinguish a successful result from a partial failure. Consider throwing or returning aResulttype.This same pattern exists in
FfmpegVideoProducerAdapter.kt(line 65).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/adapter/AndroidVideoProducerAdapter.kt` around lines 62 - 65, In AndroidVideoProducerAdapter (produceVideo) avoid silently returning outputFile when a frame save fails: instead propagate the failure by throwing a descriptive exception (or change the method to return a Result/ sealed outcome) so callers can distinguish partial/failed work; update the catch block inside produceVideo (the block logging "Error saving frame") to include throwing a custom exception (or returning Result.failure with the caught exception) rather than returning outputFile, and apply the same change to the analogous catch in FfmpegVideoProducerAdapter.kt.modules/ffmpeg-motion-ext/src/main/java/com/tejpratapsingh/motionlib/ffmpeg/FfmpegVideoProducerAdapter.kt (1)
150-152:⚠️ Potential issue | 🟠 MajorInteger division truncates audio timing values.
startFrame,endFrame, anddelayFrameare divided byfpsusing integer division, which truncates fractional seconds. For example, frame 13 at 30fps yields0seconds instead of0.433s. This will produce incorrectatrimandadelayvalues for any audio track not aligned to exact second boundaries.Proposed fix
audioTracks.forEachIndexed { index, track -> - val startSec = track.startFrame / fps - val endSec = track.endFrame / fps - val delayMs = (track.delayFrame / fps * 1000) + val startSec = track.startFrame.toDouble() / fps + val endSec = track.endFrame.toDouble() / fps + val delayMs = (track.delayFrame.toDouble() / fps * 1000).toLong()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/ffmpeg-motion-ext/src/main/java/com/tejpratapsingh/motionlib/ffmpeg/FfmpegVideoProducerAdapter.kt` around lines 150 - 152, The calculations for startSec, endSec and delayMs in FfmpegVideoProducerAdapter.kt use integer division and truncate fractions; change the expressions that compute startSec, endSec and delayMs (currently using track.startFrame / fps, track.endFrame / fps, track.delayFrame / fps * 1000) to perform floating-point division (e.g., convert numerator or fps to Double/Float) so startSec and endSec are fractional seconds and delayMs is a precise millisecond Double/Long; update any downstream use (atrim/adelay formatting) to format these fractional values appropriately.
♻️ Duplicate comments (19)
modules/app/src/main/java/com/tejpratapsingh/animator/worker/SampleMotionWorker.kt (1)
73-81:⚠️ Potential issue | 🟠 Major
Dispatchers.Unconfinedis incorrect for blocking I/O — useDispatchers.IO.
File.createTempFileis a blocking filesystem call.Dispatchers.Unconfineddoes not switch threads — it runs the block immediately on the caller's coroutine context, making thewithContextwrapper effectively a no-op. This means the blocking call executes on whatever thread the caller happens to be on (potentially the main thread or a limited WorkManager thread).Use
Dispatchers.IOto properly offload the file creation, consistent withMotionVideoContainer.ktwhich usesDispatchers.IOfor its file operations.Proposed fix
override suspend fun getOutputFile(): File = - withContext(Dispatchers.Unconfined) { + withContext(Dispatchers.IO) { File .createTempFile( "motion_video_out_", // More descriptive prefix ".mp4", applicationContext.cacheDir, ) }
Separate concern — output file in
cacheDirmay be evicted before the user acts on the notification.This was flagged in a previous review and remains unaddressed. The OS can purge cache files at any time; the notification's share/open intents will reference a stale path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/app/src/main/java/com/tejpratapsingh/animator/worker/SampleMotionWorker.kt` around lines 73 - 81, In SampleMotionWorker.override suspend fun getOutputFile(), replace withContext(Dispatchers.Unconfined) with withContext(Dispatchers.IO) so the blocking File.createTempFile call runs on the IO dispatcher (aligns with MotionVideoContainer.kt), and change the temp directory from applicationContext.cacheDir to a non-evictable location such as applicationContext.filesDir (or an app-specific external files dir) so the notification share/open intents don’t point at a file the OS can purge.modules/metadata-extractor/src/main/java/com/tejpratapsingh/motion/metadataextractor/presentation/ShareReceiverActivity.kt (1)
72-84: LGTM! Both issues from the previous review are addressed.
TAGis now used consistently and the no-link branch correctly shows aToastand callsfinish(), eliminating the infinite-spinner case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/metadata-extractor/src/main/java/com/tejpratapsingh/motion/metadataextractor/presentation/ShareReceiverActivity.kt` around lines 72 - 84, No change needed: the handleSharedText function correctly logs received text using TAG, extracts links via extractLinks, invokes metadataViewModel.getMetaData(sharedLink) for the first link, and in the no-link branch shows a Toast and calls finish(); leave handleSharedText, extractLinks usage, metadataViewModel.getMetaData, and the no-link Toast+finish behavior as-is.modules/jcodec-motion-ext/build.gradle (1)
1-4: Samekotlin.androidremoval concern — verify no Kotlin sources.Same pattern as other modules. Refer to the verification comment in
modules/metadata-extractor/build.gradle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/jcodec-motion-ext/build.gradle` around lines 1 - 4, The review warns that the kotlin.android plugin may have been removed from the plugins block; verify whether this module actually contains any Kotlin sources (check src/main/kotlin, src/test/kotlin, src/androidTest/kotlin and any Kotlin files referenced by module classes or test configs) and if Kotlin files exist re-add the kotlin("android") (or apply plugin: "kotlin-android") declaration to the plugins block so Kotlin sources are compiled; if there are no Kotlin sources remove any Kotlin-related config entries (kapt, kotlinOptions, etc.) to avoid dead config.modules/3d-opengl-renderer/build.gradle (1)
1-4: Samekotlin.androidremoval concern — verify no Kotlin sources.Same pattern as other modules. Refer to the verification comment in
modules/metadata-extractor/build.gradle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/3d-opengl-renderer/build.gradle` around lines 1 - 4, The reviewer is concerned we may have removed the kotlin.android plugin without verifying there are no Kotlin sources; check the module's source tree for any .kt/.kts files and references to Kotlin-specific Gradle config (e.g., kotlinOptions, Kotlin sourceSets, apply plugin: "kotlin-android" or usages of Kotlin JVM/Android tasks) and if none exist remove any leftover kotlin.android plugin declaration or related config; if Kotlin sources exist, restore or keep the kotlin.android plugin and adjust the plugins block (plugins { alias(libs.plugins.android.library) id 'maven-publish' } ) and any kotlin-specific config accordingly.modules/3d-filament-renderer/build.gradle (1)
1-4: Samekotlin.androidremoval concern — verify no Kotlin sources.Same pattern as other modules. Refer to the verification comment in
modules/metadata-extractor/build.gradle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/3d-filament-renderer/build.gradle` around lines 1 - 4, The plugins block currently uses alias(libs.plugins.android.library) but the reviewer flagged removal of a kotlin.android plugin—verify whether this module contains Kotlin sources (search for .kt files or Kotlin-specific source sets, or references to Kotlin classes in the module); if there are no Kotlin sources remove any kotlin.android plugin reference and related Kotlin config, otherwise keep or add the kotlin(android) plugin (e.g., ensure libs.plugins.kotlin.android is applied) and update the plugins block so the module compiles (check build scripts and source dirs referenced by the module to confirm the correct plugin is present).modules/sdui/build.gradle (1)
1-4: Samekotlin.androidremoval concern — verify no Kotlin sources.Same pattern as other modules. Refer to the verification comment in
modules/metadata-extractor/build.gradle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/sdui/build.gradle` around lines 1 - 4, The plugins block now only contains alias(libs.plugins.android.library) and id 'maven-publish' but the Kotlin Android plugin removal must be safe; verify there are no Kotlin sources before removing kotlin.android: search the module for .kt/.kts files and for src/*/kotlin directories and check Gradle/Kotlin DSL usages (build scripts, test sources, and any generated Kotlin code). If any Kotlin files or kotlin source sets exist, re-add the kotlin.android plugin; if none exist, proceed with the removal and ensure no kotlin-specific config (sourceSets, kotlinOptions) remains in this module’s Gradle config.modules/motion-store/build.gradle (1)
1-4: Past concerns about missingkotlin-androidplugin and invalidcompileSdksyntax are now resolved.AGP 9.0 introduces built-in Kotlin support and enables it by default —
org.jetbrains.kotlin.androidno longer needs to be applied to compile Kotlin source files. ThecompileSdk 36syntax is also now correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/build.gradle` around lines 1 - 4, The previous concerns about missing kotlin-android and compileSdk syntax are resolved; keep the plugins block using alias(libs.plugins.android.library) and id 'maven-publish' as-is, and do not add org.jetbrains.kotlin.android (kotlin-android) back since AGP 9+ provides built-in Kotlin support; ensure no redundant kotlin plugin is applied elsewhere and leave the compileSdk declaration using the modern integer syntax (e.g., compileSdk 36) intact.gradle/libs.versions.toml (1)
21-23: Past concern about Kotlin 2.3.0 + AGP 9.0.0 incompatibility is resolved.The upgrade to
kotlinVersion = "2.3.10"addresses the prior review comment. Note the separate AGP patch-version finding above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle/libs.versions.toml` around lines 21 - 23, The previous incompatibility is resolved by bumping kotlinVersion to "2.3.10"; keep the updated entries espresso_core_version, agpVersion, and kotlinVersion as-is and remove the stray "[duplicate_comment]" marker from the file so the TOML contains only the version keys (espresso_core_version, agpVersion, kotlinVersion) without the duplicate note.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/lrclib/client/LrcLibApiServiceImpl.kt (1)
68-70:catch (e: Exception)still swallowsCancellationException, breaking structured concurrency.Also applies to: 116-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/lrclib/client/LrcLibApiServiceImpl.kt` around lines 68 - 70, The current catch blocks in LrcLibApiServiceImpl (where you return Result.failure(e)) catch Exception and thus swallow CancellationException; modify those catch clauses so CancellationException is rethrown (e.g., use catch (e: Exception) { if (e is CancellationException) throw e; return Result.failure(e) } or use catch (e: CancellationException) { throw e } plus a separate catch for other Exceptions) to preserve structured concurrency while still converting other exceptions to Result.failure; update both occurrences surrounding the try that calls Result.failure to follow this pattern.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SyncedLyricsSelector.kt (1)
586-591: Integer division still truncates frame times;provideCurrentConfig()still called per item.
line.frame / provideCurrentConfig().fpsperforms integer division (Int / Int), so frames shorter than onefpsinterval display as0 sec. Additionally,provideCurrentConfig()is invoked on every list item during every recomposition instead of receiving thefpsvalue already hoisted at line 183.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SyncedLyricsSelector.kt` around lines 586 - 591, The displayed seconds use integer division and call provideCurrentConfig() per item; change the Text content to compute a floating-point seconds value using line.frame.toDouble() / fps (use the hoisted fps variable already available) and format it (e.g., to 2 decimal places) instead of calling provideCurrentConfig() inside each item, so replace "[${line.frame / provideCurrentConfig().fps} sec]" with a formatted string built from line.frame.toDouble() / fps.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/lrclib/model/LyricsResponse.kt (1)
7-7: Unused importkotlinx.serialization.Serializableis still present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/lrclib/model/LyricsResponse.kt` at line 7, Remove the unused import kotlinx.serialization.Serializable from LyricsResponse.kt; locate the import statement for kotlinx.serialization.Serializable and delete it (or if the data class/model LyricsResponse is intended to be serializable, annotate the class with `@Serializable` and keep the import), ensuring there are no remaining references to kotlinx.serialization.Serializable after the change.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt (1)
153-159:"Success ${lyricsState.lyrics.size}"toast is a debug placeholder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt` around lines 153 - 159, Replace the debug placeholder toast in SearchActivity's LyricsUiState.Success branch: instead of Toast.makeText(this@SearchActivity, "Success ${lyricsState.lyrics.size}", ...), show a user-facing message or update the UI appropriately (e.g., display a localized success string or update the RecyclerView/empty-state based on lyricsState.lyrics.isEmpty). Modify the LyricsUiState.Success handling (in SearchActivity) to remove the debug interpolation of lyricsState.lyrics.size and use a proper string resource or UI update method so users see meaningful feedback.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt (1)
87-88:selectedSongNamestill produces"null - null"whenselectedLyricis unset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt` around lines 87 - 88, selectedSongName currently concatenates nullable properties directly from selectedLyric.value causing "null - null" when unset; update the getter to handle nulls by returning a safe placeholder or empty string. Locate the selectedSongName property in LyricsViewModel and change the expression using safe operators (e.g., selectedLyric.value?.trackName ?: "" and selectedLyric.value?.artistName ?: "") or compute via selectedLyric.value?.let { "${it.trackName} - ${it.artistName}" } ?: "" so it never yields "null - null".modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt (1)
89-97:songDetails[1]still throwsIndexOutOfBoundsExceptionwhensongNamehas no" - "separator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt` around lines 89 - 97, The code in LyricsContainer.kt splits songName into songDetails and directly accesses songDetails[1], which throws IndexOutOfBoundsException when there is no " - " separator; update the logic in the block that prepares url (the lines using songName, songDetails, and calling repository.getAlbumArtUrl) to safely handle missing separator by either using split with a limit and checking songDetails.size before accessing [1], or deriving artist/title with safe fallbacks (e.g., treat entire songName as title and pass an empty/unknown artist) before calling repository.getAlbumArtUrl, and only call setImageBitmap when repository.getAlbumArtBitmap returns a bitmap.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SearchLyricsCompose.kt (1)
149-179: Error state is silently swallowed by theelsebranch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SearchLyricsCompose.kt` around lines 149 - 179, The current else branch in SearchLyricsCompose.kt silently swallows the error state instead of showing it; update the UI state handling in the composable that uses recentSearches so you explicitly handle the error case (e.g., when(viewModel.uiState) { is Error -> show an error Text or retry Button }, is Loading -> show progress, is Success/Empty -> show recentSearches LazyColumn). Locate and modify the block that builds the recent searches list (references: recentSearches, viewModel.query.tryEmit, viewModel.searchLyrics, LazyColumn) to replace the generic else with explicit branches for the Error state and render an appropriate error message/action. Ensure keyboardController hiding and click behavior remain in the recent item click handler.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/albumart/client/AlbumArtRemoteDataSourceImpl.kt (2)
69-79:fetchBitmapis still missing exception handling — still outstanding from previous review.
client.newCall(request).execute()can throwIOExceptionon network failure; without atry/catchthe exception propagates to the caller instead of returningnull.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/albumart/client/AlbumArtRemoteDataSourceImpl.kt` around lines 69 - 79, The fetchBitmap function should catch network/IO exceptions so failures return null instead of propagating; wrap the client.newCall(request).execute() and the response handling inside a try/catch for IOException (and optionally Exception), return null from the catch block, and optionally log the error via your logger; ensure you still use response.use { ... } and BitmapFactory.decodeByteArray(bytes, 0, bytes.size) inside the try so resources are closed correctly.
31-34: Unescaped Lucene special characters in query — still outstanding from previous review.
trackNameandartistNameare still interpolated without Lucene escaping. Characters like",+,-,(,),\will malform the query.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/albumart/client/AlbumArtRemoteDataSourceImpl.kt` around lines 31 - 34, The query string in AlbumArtRemoteDataSourceImpl is interpolating trackName and artistName directly into a Lucene query (the addQueryParameter call building "recording:\"$trackName\" AND artist:\"$artistName\""), which allows unescaped Lucene special characters to break the query; add a helper (e.g., escapeLuceneSpecialChars(String) or similar) that escapes Lucene special characters (e.g., + - && || ! ( ) { } [ ] ^ " ~ * ? : \ / and whitespace rules) by prefixing them with a backslash, apply this helper to trackName and artistName before building the query, and use the escaped values in the addQueryParameter call to ensure well-formed queries.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.kt (1)
92-96: In-memoryProjectStorestate is lost on WorkManager process restart — still outstanding from previous review.If the process is killed while work is enqueued, WorkManager restarts the worker in a fresh process where
ProjectStore.motionProjectisnull, causingprovideCurrentProject()to generate a new random-UUID project disconnected from the intended one. The project ID should be persisted ininputData.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.kt` around lines 92 - 96, The worker loses in-memory ProjectStore.motionProject on process restart; persist and restore the project ID via WorkManager inputData so provideCurrentProject() doesn't create a new project: when scheduling the work write the current project's UUID into the WorkRequest inputData, then in LyricsMotionWorker.getOutputFile() (or inside provideCurrentProject()) read that projectId from inputData and use it to rehydrate or fetch the existing motionProject instead of letting provideCurrentProject() generate a new random UUID; ensure createProjectFile(applicationContext.createProjectFile(motionProject)) receives the restored project instance.modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/MotionProject.kt (1)
24-32: TOCTOU race inprovideCurrentProject— still outstanding from previous review.
UUID.randomUUID().toString()is evaluated as a default argument on every call even whenProjectStore.motionProjectis already populated. The read-check-set sequence on the@Volatilefield is also non-atomic: two concurrent callers can both observenulland create distinctMotionProjectinstances with different UUIDs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/MotionProject.kt` around lines 24 - 32, The current provideCurrentProject function uses a default argument UUID.randomUUID().toString() and a non-atomic read-check-set on ProjectStore.motionProject, causing a TOCTOU race where concurrent callers can create different MotionProject instances; fix by removing the default UUID generation from the parameter and instead generate the id only when actually creating a new MotionProject inside an atomic/locked creation path (e.g., synchronize on ProjectStore or use an AtomicReference compare-and-set) so that the read-check-set for ProjectStore.motionProject in provideCurrentProject is performed atomically and only one MotionProject (with one UUID) is created.
🟡 Minor comments (7)
modules/metadata-extractor/src/main/res/layout/activity_share_receiver.xml-40-42 (1)
40-42:⚠️ Potential issue | 🟡 Minor
actionNextis semantically incorrect —tv_titleis the only input field in this layout.
android:imeOptions="actionNext"is intended to move focus to the next focusable input. Sincetv_titleis the soleTextInputEditTextin the layout, pressing "Next" on the software keyboard will not navigate anywhere meaningful (the focus may jump tobtn_nextor simply not respond), leaving the keyboard open.actionDonedismisses the keyboard and better matches the user's intent after entering a title.🛠️ Proposed fix
- android:imeOptions="actionNext" + android:imeOptions="actionDone"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/metadata-extractor/src/main/res/layout/activity_share_receiver.xml` around lines 40 - 42, The tv_title TextInputEditText uses android:imeOptions="actionNext" but it's the only input in the layout; update the tv_title element to use android:imeOptions="actionDone" instead so the IME sends a Done action (dismissing the keyboard) after entering the title; locate the tv_title view in activity_share_receiver.xml and replace the imeOptions value accordingly.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/lrclib/model/LyricsResponse.kt-22-23 (1)
22-23:⚠️ Potential issue | 🟡 Minor
duration: Doublesilently maps missing/null JSON values to0.0.When Gson deserializes a response that omits
durationor sets it tonull, it assigns the JVM primitive default (0.0) to a non-nullableDoublefield without any error. The previousFloat? = nullmodel surfaced this absence explicitly. If the LRCLib API can return responses withoutduration(e.g., for some instrumental entries),getReadableDuration()will silently report "00 min and 00 sec" instead of indicating missing data.Consider keeping
duration: Double? = nulland updatinggetReadableDuration()with a null guard, or accepting the implicit default if the field is guaranteed by the API contract.🛡️ Proposed fix (if null duration is possible)
- `@SerializedName`("duration") - val duration: Double, + `@SerializedName`("duration") + val duration: Double?,fun getReadableDuration(): String { - val totalSeconds = duration.toInt() + val totalSeconds = (duration ?: 0.0).toInt() val minutes = totalSeconds / 60 val seconds = totalSeconds % 60 return String.format(Locale.getDefault(), "%02d min and %02d sec", minutes, seconds) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/lrclib/model/LyricsResponse.kt` around lines 22 - 23, The model currently declares duration: Double which silently becomes 0.0 when JSON omits/nulls the field; change the property in LyricsResponse (duration) to a nullable type (Double? = null) and update getReadableDuration() to handle null (e.g., return a sentinel like "unknown" or skip formatting) so missing data is explicit; ensure any callers of getReadableDuration() or direct accesses to duration are adjusted for the nullable type (use safe calls or null-checks).modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt-182-182 (1)
182-182:⚠️ Potential issue | 🟡 MinorUnnecessary parentheses around
retrySeconds - 1.
-has higher precedence than the infixdownTo, soretrySeconds - 1 downTo 1parses identically.🐛 Proposed fix
- for (i in (retrySeconds - 1) downTo 1) { + for (i in retrySeconds - 1 downTo 1) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt` at line 182, In SearchActivity.kt the for-loop uses unnecessary parentheses in "for (i in (retrySeconds - 1) downTo 1)"; remove the redundant parentheses so it reads "for (i in retrySeconds - 1 downTo 1)" (no behavior change) to improve readability around the expression in the loop within the method containing this retry logic.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt-179-186 (1)
179-186:⚠️ Potential issue | 🟡 Minor
negativeButtonholds a reference toBUTTON_POSITIVE.The variable is named
negativeButtonbut is retrieved viaDialogInterface.BUTTON_POSITIVE. While the countdown logic is functionally correct (it updates the retry/positive button's text), the misleading name is confusing.🐛 Proposed fix
- val negativeButton = errorDialog.getButton(DialogInterface.BUTTON_POSITIVE) + val retryButton = errorDialog.getButton(DialogInterface.BUTTON_POSITIVE) for (i in (retrySeconds - 1) downTo 1) { delay(1000) if (errorDialog.isShowing) { - negativeButton.text = getString(R.string.retry, i) + retryButton.text = getString(R.string.retry, i) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt` around lines 179 - 186, The local variable negativeButton is misnamed: it is assigned from errorDialog.getButton(DialogInterface.BUTTON_POSITIVE) inside the lifecycleScope.launch countdown (using retrySeconds) which is confusing; rename the variable to positiveButton (or fetch DialogInterface.BUTTON_NEGATIVE if you really meant the negative action) and update all references in that block (the variable declaration and the subsequent positiveButton.text assignment) so the name matches the actual button being manipulated.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.kt-55-60 (1)
55-60:⚠️ Potential issue | 🟡 MinorWake lock is created but never acquired — dead scaffolding.
wakeLockis initialized at class level butwakeLock.acquire()andwakeLock.release()are never called anywhere, so the CPU can still sleep during rendering. The tag"MyApp::MyWakelockTag"is also a placeholder. Either wire the acquire/release around the render lifecycle or remove the scaffolding until it's needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.kt` around lines 55 - 60, The wakeLock is allocated but never used; either remove the scaffolding or actually acquire/release it around the render lifecycle. Locate the PowerManager and WakeLock fields (powerManager and wakeLock) in LyricsMotionWorker and either delete them, or wrap the work execution: call wakeLock.acquire(timeoutMillis) at the start of the worker’s execution method (e.g., doWork or startWork) and ensure wakeLock.release() in a finally block or in onStopped to always release; replace the placeholder tag "MyApp::MyWakelockTag" with a descriptive tag such as "${appContext.packageName}:LyricsMotionWorker" and ensure you check wakeLock.isHeld before releasing to avoid IllegalStateException.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.kt-245-245 (1)
245-245:⚠️ Potential issue | 🟡 MinorWrong
TAGconstant — copy-paste artifact.
TAGis set to"SampleMotionWorker"insideLyricsMotionWorker, so all logs from this class appear under the wrong tag.🐛 Proposed fix
- private const val TAG = "SampleMotionWorker" + private const val TAG = "LyricsMotionWorker"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.kt` at line 245, The TAG constant in LyricsMotionWorker is a copy-paste artifact ("SampleMotionWorker") causing incorrect log tagging; update the private const val TAG in the LyricsMotionWorker class to the correct identifier (e.g., "LyricsMotionWorker") or derive it from the class name (e.g., using LyricsMotionWorker::class.java.simpleName) so logs are emitted under the correct tag.modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/BaseDao.kt-88-88 (1)
88-88:⚠️ Potential issue | 🟡 MinorMisleading doc:
insertOrThrownever returns -1 — it throws.The KDoc says "Returns the new row ID, or -1 on failure," but
insertOrThrowthrows an exception on failure rather than returning -1. Update the doc to reflect the actual behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/BaseDao.kt` at line 88, The KDoc for BaseDao.insert is incorrect: it says "Returns the new row ID, or -1 on failure" but the implementation calls SQLiteDatabase.insertOrThrow which throws on failure. Update the KDoc for the insert(T): Long method to state it returns the new row ID and will throw an exception (e.g., SQLException) on failure instead of returning -1, and mention any thrown exceptions if you want to be explicit.
| <project version="4"> | ||
| <component name="ExternalStorageConfigurationManager" enabled="true" /> | ||
| <component name="ProjectRootManager" version="2" languageLevel="JDK_17" default="true" project-jdk-name="jbr-17" project-jdk-type="JavaSDK"> | ||
| <component name="ProjectRootManager" version="2" languageLevel="JDK_21" default="true" project-jdk-name="jbr-21" project-jdk-type="JavaSDK"> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Gradle build files target Java 21 consistently
echo "=== Checking sourceCompatibility / targetCompatibility ==="
rg -n 'sourceCompatibility|targetCompatibility' --type groovy --type kotlin -C 2
echo ""
echo "=== Checking Kotlin jvmTarget ==="
rg -n 'jvmTarget' --type groovy --type kotlin -C 2
echo ""
echo "=== Checking toolchain / kotlin.jvm.toolchain ==="
rg -n 'javaVersion|toolchain' --type groovy --type kotlin -C 2Repository: tejpratap46/AndroidVideoMotion
Length of output: 5887
Gradle build files do not match the JDK 21 target in .idea/misc.xml—this change must be reverted or all Gradle files must be updated.
The IDE config was changed to JDK 21, but the Gradle build scripts remain on Java 11 (most modules) or Java 8 (modules/templates, modules/motionlib, modules/app). This mismatch will cause developers to see JDK 21 in the IDE while the build targets an older JVM version.
Either revert .idea/misc.xml to align with the actual Gradle config, or update all compileOptions { sourceCompatibility JavaVersion.VERSION_11 } blocks to JavaVersion.VERSION_21 across all modules.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.idea/misc.xml at line 3, The .idea ProjectRootManager was changed to
languageLevel="JDK_21"/project-jdk-name="jbr-21" but Gradle still targets older
Java versions; either revert that IDE change or update Gradle compile options.
Restore ProjectRootManager's languageLevel/project-jdk-name back to the previous
JDK if you want to keep Gradle as-is, or update every Gradle module that uses
compileOptions { sourceCompatibility JavaVersion.VERSION_11 } (and any
JavaVersion.VERSION_1_8 entries in modules/templates, modules/motionlib,
modules/app) to JavaVersion.VERSION_21 and corresponding targetCompatibility
entries so the IDE JDK and Gradle build targets match.
| agpVersion = "9.0.1" | ||
| kotlinVersion = "2.3.10" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
AGP 9.0.28 Kotlin 2.3 support Android Gradle Plugin compatibility
💡 Result:
- Yes — Kotlin 2.3 is supported with AGP 9.x starting at AGP 9.0.28. Android’s Kotlin support table explicitly notes: “9.x versions before 9.0.28 don't support Kotlin 2.3.” [1]
- With AGP 9.0, Kotlin is built in (enabled by default), and you should not apply
org.jetbrains.kotlin.android/kotlin-android. AGP 9.0 also has a runtime dependency on Kotlin Gradle Plugin (KGP) 2.2.10 for this built-in Kotlin integration. [2] - Kotlin 2.3 also documents that
kotlin-androidis deprecated with AGP 9.0.0+ because AGP provides built-in Kotlin. [3]
Practical takeaway: If you want Kotlin 2.3 + AGP 9, use AGP ≥ 9.0.28 and migrate to AGP’s built-in Kotlin (remove kotlin-android). [1][2][3]
Sources:
[1] Android Developers — “AGP, D8, and R8 versions required for Kotlin versions” (footnote about 9.0.28) (developer.android.com)
[2] Android Developers — “Android Gradle plugin 9.0.0 …” (built-in Kotlin; KGP 2.2.10 runtime dependency) (developer.android.com)
[3] Kotlin Docs — “Compatibility guide for Kotlin 2.3” (kotlin-android deprecated with AGP 9.0+) (kotlinlang.org)
Citations:
- 1: https://developer.android.com/build/kotlin-support?utm_source=openai
- 2: https://developer.android.com/tools/revisions/gradle-plugin.html?utm_source=openai
- 3: https://kotlinlang.org/docs/compatibility-guide-23.html?utm_source=openai
🌐 Web query:
Android Gradle Plugin 9.0.1 Kotlin 2.3.10 compatibility D8 R8
💡 Result:
- Kotlin 2.3.x (including 2.3.10) requires at least AGP 8.13.2 and R8 8.13.19 (minimums). Google’s compatibility table also notes that R8 9.x versions before 9.0.28 don’t support Kotlin 2.3. [1]
- Kotlin Multiplatform plugin 2.3.10’s published compatibility range lists AGP up to 9.0.0 (not 9.0.1). So AGP 9.0.1 + Kotlin 2.3.10 is not a “known-good” combo from JetBrains’ matrix. [2]
What to do
- Preferred: use an AGP version that bundles an R8/D8 combo known to support Kotlin 2.3 (per Google’s table), or upgrade AGP until it bundles R8 ≥ 9.0.28. [1]
- If you must stay on AGP 9.0.1: override the embedded R8 with a newer R8 (≥ 9.0.28) using the documented “replace R8 in AGP” mechanism. [3]
Sources:
[1] Google Android Developers – “AGP, D8, and R8 versions required for Kotlin versions” (incl. Kotlin 2.3 row + note about R8 9.0.28)
[2] JetBrains Kotlin docs – Multiplatform compatibility guide (Kotlin 2.3.10 AGP range)
[3] R8 (googlesource) – “Replacing R8 in Android Gradle plugin” instructions
🌐 Web query:
Android Gradle Plugin release notes Kotlin 2.3 support minimum version
💡 Result:
For Kotlin 2.3 bytecode/metadata support in the Android toolchain, Google’s compatibility table lists:
- Minimum Android Gradle Plugin (AGP): 8.13.2 (with R8 8.13.19) for Kotlin 2.3. [1]
- If you’re on AGP 9.x, note the footnote: AGP 9.x versions earlier than 9.0.28 don’t support Kotlin 2.3 (so use ≥ 9.0.28). [1]
Separately (from the Kotlin Gradle plugin side), Kotlin 2.3.0 states the minimum supported AGP is 8.2.2 (compatibility range guidance, not the “minimum required for Kotlin 2.3 class files” table). [2]
Sources:
[1] Android Developers — “AGP, D8, and R8 versions required for Kotlin versions”
[2] Kotlin docs — “What’s new in Kotlin 2.3.0”
AGP 9.0.1 does not support Kotlin 2.3.x — upgrade AGP to 9.0.28 or higher.
AGP 9.x versions before 9.0.28 don't support Kotlin 2.3. The pairing of agpVersion = "9.0.1" with kotlinVersion = "2.3.10" is incompatible and can cause D8/R8 build failures. Upgrade agpVersion to 9.0.28 or later.
Proposed fix
-agpVersion = "9.0.1"
+agpVersion = "9.0.28"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| agpVersion = "9.0.1" | |
| kotlinVersion = "2.3.10" | |
| agpVersion = "9.0.28" | |
| kotlinVersion = "2.3.10" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gradle/libs.versions.toml` around lines 22 - 23, The agpVersion and
kotlinVersion pairing is incompatible—update the agpVersion value used in
libs.versions.toml (agpVersion = "9.0.1") to at least "9.0.28" (or a newer AGP
9.x release) so it supports kotlinVersion = "2.3.10"; locate the agpVersion
entry in gradle/libs.versions.toml and change it to "9.0.28" (or a later
compatible AGP) to resolve the D8/R8 build failures.
| lifecycleScope.launch { | ||
| repeatOnLifecycle(Lifecycle.State.STARTED) { | ||
| lyricsViewModel.uiState.collect { | ||
| handleLyricsSearch(it) | ||
| } | ||
| } | ||
|
|
||
| repeatOnLifecycle(Lifecycle.State.STARTED) { | ||
| projectsViewModel.shareEvent.collect { | ||
| shareProjectFile(it) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Second repeatOnLifecycle is unreachable — projectsViewModel.shareEvent is never collected.
repeatOnLifecycle suspends the coroutine indefinitely (restarting each time the lifecycle crosses STARTED) and only resumes when the lifecycle reaches DESTROYED. Because both calls are sequential inside the same launch block, the second repeatOnLifecycle at line 111 can never execute, so projectsViewModel.shareEvent is silently dropped and the share-project path through onProjectClick is broken.
🐛 Proposed fix — use separate `launch` blocks
- lifecycleScope.launch {
- repeatOnLifecycle(Lifecycle.State.STARTED) {
- lyricsViewModel.uiState.collect {
- handleLyricsSearch(it)
- }
- }
-
- repeatOnLifecycle(Lifecycle.State.STARTED) {
- projectsViewModel.shareEvent.collect {
- shareProjectFile(it)
- }
- }
- }
+ lifecycleScope.launch {
+ repeatOnLifecycle(Lifecycle.State.STARTED) {
+ lyricsViewModel.uiState.collect {
+ handleLyricsSearch(it)
+ }
+ }
+ }
+ lifecycleScope.launch {
+ repeatOnLifecycle(Lifecycle.State.STARTED) {
+ projectsViewModel.shareEvent.collect {
+ shareProjectFile(it)
+ }
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt`
around lines 104 - 116, The second repeatOnLifecycle is never reached because
both repeatOnLifecycle calls are inside the same lifecycleScope.launch and the
first suspends indefinitely; split the collectors into separate coroutines so
both flows are collected. Specifically, in SearchActivity.kt move the
projectsViewModel.shareEvent.collect block into its own lifecycleScope.launch {
repeatOnLifecycle(Lifecycle.State.STARTED) {
projectsViewModel.shareEvent.collect { shareProjectFile(it) } } } (keeping the
existing lifecycleScope.launch/repeatOnLifecycle for
lyricsViewModel.uiState.collect and handleLyricsSearch(it)) so both collectors
run concurrently.
| }.setPositiveButton(retryText) { dialog, _ -> | ||
| dialog.dismiss() | ||
| lyricsViewModel.searchLyrics(lyricsViewModel.query.value) | ||
| }.show() |
There was a problem hiding this comment.
Retry call maps query.value to trackName instead of query, inconsistent with the original search.
lyricsViewModel.searchLyrics(lyricsViewModel.query.value) is a positional argument, mapping to the first parameter trackName. The keyboard-triggered search (via SearchLyricsCompose) calls searchLyrics(query = searchQuery) using the q free-text field. On retry, this difference causes the LrcLib API to search by exact track name rather than by the same free-text query, which can return different results.
🐛 Proposed fix
- lyricsViewModel.searchLyrics(lyricsViewModel.query.value)
+ lyricsViewModel.searchLyrics(query = lyricsViewModel.query.value)Apply the same fix at line 192 (the auto-retry after the countdown).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }.setPositiveButton(retryText) { dialog, _ -> | |
| dialog.dismiss() | |
| lyricsViewModel.searchLyrics(lyricsViewModel.query.value) | |
| }.show() | |
| }.setPositiveButton(retryText) { dialog, _ -> | |
| dialog.dismiss() | |
| lyricsViewModel.searchLyrics(query = lyricsViewModel.query.value) | |
| }.show() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt`
around lines 174 - 177, Retry currently calls
lyricsViewModel.searchLyrics(lyricsViewModel.query.value) which passes the value
as the positional first parameter (trackName) causing a different search
behavior; change the retry calls (both in the dialog positive button and the
auto-retry after countdown) to call the ViewModel using the named parameter used
by the keyboard-triggered search (e.g., lyricsViewModel.searchLyrics(query =
lyricsViewModel.query.value)) so the free-text q field is used consistently;
update both references (the dialog block using setPositiveButton and the
auto-retry block around line 192) to use the named argument searchLyrics(query =
...) instead of the positional call.
| var showDeleteDialog by remember { mutableStateOf(false) } | ||
|
|
||
| val context = LocalContext.current | ||
| val thumbnail: Bitmap? = remember(project.id) { extractFirstFrame(context.createProjectFile(project).path) } |
There was a problem hiding this comment.
extractFirstFrame performs disk + media I/O on the main/composition thread.
remember(project.id) { extractFirstFrame(...) } runs synchronously during composition. MediaMetadataRetriever.setDataSource and getFrameAtTime are both blocking I/O operations that can cause noticeable jank, especially in a grid with many projects.
Move this to a LaunchedEffect with Dispatchers.IO or use produceState:
Proposed fix using produceState
- val thumbnail: Bitmap? = remember(project.id) { extractFirstFrame(context.createProjectFile(project).path) }
+ val thumbnail: Bitmap? by produceState<Bitmap?>(initialValue = null, project.id) {
+ value = withContext(Dispatchers.IO) {
+ extractFirstFrame(context.createProjectFile(project).path)
+ }
+ }This requires adding imports for produceState, withContext, and Dispatchers:
import androidx.compose.runtime.produceState
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.withContext🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectsScreenCompose.kt`
at line 236, The composition currently calls extractFirstFrame synchronously via
remember(project.id) which performs blocking disk/media I/O
(MediaMetadataRetriever) on the main/composition thread; change this to run off
the main thread by replacing the remember(...) usage with a coroutine-based
state producer (e.g., produceState or LaunchedEffect) that calls
extractFirstFrame inside a withContext(Dispatchers.IO) block and assigns the
resulting Bitmap to the composable state; update imports for
produceState/LaunchedEffect, withContext, and Dispatchers, and ensure you still
key by project.id when launching the work so thumbnails load asynchronously
without janking UI.
| // Default: drop all tables and recreate | ||
| // Disable FK constraints during migration to avoid constraint errors on drop | ||
| db.execSQL("PRAGMA foreign_keys = OFF") | ||
| schemas.forEach { schema -> | ||
| val tableName = | ||
| schema | ||
| .substringAfter("CREATE TABLE IF NOT EXISTS ", "") | ||
| .substringAfter("CREATE TABLE ", "") | ||
| .substringBefore(" ") | ||
| .trim() | ||
| if (tableName.isNotEmpty()) db.execSQL("DROP TABLE IF EXISTS $tableName") | ||
| } | ||
| db.execSQL("PRAGMA foreign_keys = ON") | ||
| onCreate(db) | ||
| } |
There was a problem hiding this comment.
PRAGMA foreign_keys = OFF/ON is a no-op inside the onUpgrade transaction.
PRAGMA foreign_keys is a no-op within a transaction; foreign key constraint enforcement may only be enabled or disabled when there is no pending BEGIN or SAVEPOINT. Android's SQLiteOpenHelper wraps onUpgrade in an explicit beginTransaction() / endTransaction() block, so both PRAGMA calls are silently ignored. FK constraints remain active (enforced by onConfigure). Once the table-name extraction bug above is fixed, any DROP TABLE against a parent table with FK-referenced children will throw a constraint error.
The workaround is to set PRAGMA foreign_keys = OFF before the transaction begins — but SQLiteOpenHelper owns that transaction boundary. A practical alternative for the default upgrade path is to use PRAGMA defer_foreign_keys = ON at the start of onUpgrade, which defers FK checks until the transaction commits:
🐛 Suggested approach
override fun onUpgrade(db: SQLiteDatabase, oldVersion: Int, newVersion: Int) {
if (onUpgradeCallback != null) {
onUpgradeCallback.invoke(db, oldVersion, newVersion)
} else {
- db.execSQL("PRAGMA foreign_keys = OFF")
+ // foreign_keys pragma is a no-op inside a transaction (SQLiteOpenHelper wraps
+ // onUpgrade in BEGIN/END). Use defer_foreign_keys instead — it defers FK
+ // checks to commit time so mid-transaction drops of parent tables don't fail.
+ db.execSQL("PRAGMA defer_foreign_keys = ON")
schemas.forEach { schema ->
// ... extract tableName and DROP ...
}
- db.execSQL("PRAGMA foreign_keys = ON")
onCreate(db)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DatabaseManager.kt`
around lines 46 - 60, The PRAGMA foreign_keys toggles are no-ops inside the
onUpgrade transaction; change the migration to call db.execSQL("PRAGMA
defer_foreign_keys = ON") at the start of onUpgrade (before any DROP/CREATE) and
remove the db.execSQL("PRAGMA foreign_keys = OFF") / ON calls; keep the existing
loop that computes tableName from schemas and perform DROP TABLE IF EXISTS while
relying on defer_foreign_keys so foreign key checks are deferred until commit.
Ensure the defer PRAGMA is executed in the same onUpgrade method (before any
DDL) and that onCreate(db) still runs after the drops.
| schemas.forEach { schema -> | ||
| val tableName = | ||
| schema | ||
| .substringAfter("CREATE TABLE IF NOT EXISTS ", "") | ||
| .substringAfter("CREATE TABLE ", "") | ||
| .substringBefore(" ") | ||
| .trim() | ||
| if (tableName.isNotEmpty()) db.execSQL("DROP TABLE IF EXISTS $tableName") | ||
| } |
There was a problem hiding this comment.
Table name extraction always produces an empty string — DROP TABLE never executes in the default upgrade path.
The two chained .substringAfter(..., "") calls are mutually exclusive rather than cascading:
- For
"CREATE TABLE IF NOT EXISTS foo (...)":
Step 1 →"foo (...)", Step 2 →""(because"CREATE TABLE "is not found in"foo (...)"; the missing-delimiter default""is returned). - For
"CREATE TABLE foo (...)":
Step 1 →""(prefix not found), Step 2 →""(not found in"").
In both cases tableName is always "", so db.execSQL("DROP TABLE IF EXISTS $tableName") is never reached. When onUpgrade then calls onCreate, every CREATE TABLE IF NOT EXISTS is silently skipped because the old tables still exist. Any schema version bump produces a silent no-op migration, leaving the DB stuck on the old schema.
🐛 Proposed fix
- val tableName =
- schema
- .substringAfter("CREATE TABLE IF NOT EXISTS ", "")
- .substringAfter("CREATE TABLE ", "")
- .substringBefore(" ")
- .trim()
- if (tableName.isNotEmpty()) db.execSQL("DROP TABLE IF EXISTS $tableName")
+ val afterKeyword = when {
+ schema.contains("CREATE TABLE IF NOT EXISTS ") ->
+ schema.substringAfter("CREATE TABLE IF NOT EXISTS ")
+ schema.contains("CREATE TABLE ") ->
+ schema.substringAfter("CREATE TABLE ")
+ else -> ""
+ }
+ val tableName = afterKeyword.substringBefore("(").trim()
+ .split(Regex("\\s+")).firstOrNull().orEmpty()
+ if (tableName.isNotEmpty()) db.execSQL("DROP TABLE IF EXISTS $tableName")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DatabaseManager.kt`
around lines 49 - 57, The table-name extraction in the schemas.forEach block
incorrectly chains substringAfter calls so tableName becomes empty; update the
logic in the schemas.forEach (where tableName is computed and used in
db.execSQL("DROP TABLE IF EXISTS $tableName")) to robustly parse both "CREATE
TABLE IF NOT EXISTS <name>" and "CREATE TABLE <name>" cases — e.g., first check
for "CREATE TABLE IF NOT EXISTS" and fallback to "CREATE TABLE", then
substringBefore the first whitespace/parenthesis and trim to produce a non-empty
tableName before calling db.execSQL; ensure this change fixes the onUpgrade path
that later calls onCreate.
| suspend fun produceVideo( | ||
| context: Context, | ||
| outputFile: File, | ||
| progressListener: ((progress: Int, bitmap: Bitmap) -> Unit)? = null, | ||
| progressListener: (suspend (progress: Int, bitmap: Bitmap) -> Unit)? = null, | ||
| ): File |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -name "MotionVideoContainer.kt"Repository: tejpratap46/AndroidVideoMotion
Length of output: 244
🏁 Script executed:
cat -n modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/IMotionVideoProducer.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 730
🏁 Script executed:
fd -type f -name "IMotionVideoProducer.kt"Repository: tejpratap46/AndroidVideoMotion
Length of output: 244
🌐 Web query:
Kotlin suspend function type compatibility implicit coercion non-suspend function type
💡 Result:
In Kotlin, suspend function types are not compatible with (and are not subtypes of) non-suspend function types, so there is no implicit coercion between them.
- A suspending function has type
suspend (A1, …, An) -> R, and per the Kotlin spec this type is “unrelated by subtyping to any non-suspending function type”. So you can’t assign/cast asuspend (T) -> Rto(T) -> R(or vice‑versa) without an explicit wrapper. (kotlinlang.org) - This is why
let(which expects(T) -> R) rejects a reference to asuspendfunction: it’s a plain type mismatch, not something the compiler will adapt. (stackoverflow.com) - Lambdas are special only in that they can be inferred as suspend or non-suspend based on context, because they don’t have an explicitly declared function type until inference picks one. But without a suspend-typed expected type, the compiler won’t “upgrade” a lambda to suspend just because it contains a suspend call. (kotlinlang.org)
- Related “escape hatch”: non-suspending
inlinelambda parameters may still be allowed to contain suspension points when the inline function is called from a suspending function (a specific exception in the coroutines/type-coloring rules). (kotlinlang.org)
Practical consequence: if an API expects (T) -> R, you must either (1) use a non-suspend function, or (2) change/use an API that accepts suspend (T) -> R, or (3) explicitly wrap/bridge (e.g., call the suspend function inside a coroutine/runBlocking, depending on your threading needs).
Citations:
- 1: https://kotlinlang.org/spec/kotlin-spec.html?utm_source=openai
- 2: https://stackoverflow.com/questions/65518876/suspend-function-reference-as-parameter-of-let-gets-error?utm_source=openai
- 3: https://kotlinlang.org/spec/kotlin-spec.html?utm_source=openai
- 4: https://kotlinlang.org/spec/asynchronous-programming-with-coroutines.html?utm_source=openai
🏁 Script executed:
find . -name "MotionVideoContainer.kt"Repository: tejpratap46/AndroidVideoMotion
Length of output: 161
🏁 Script executed:
rg -t kotlin "produceVideo" --context 5Repository: tejpratap46/AndroidVideoMotion
Length of output: 18938
🏁 Script executed:
find . -type f -name "*.kt" | xargs grep -l "progressListener"Repository: tejpratap46/AndroidVideoMotion
Length of output: 854
🏁 Script executed:
sed -n '1,160p' ./modules/app/src/main/java/com/tejpratapsingh/animator/ui/view/MotionVideoContainer.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 4448
MotionVideoContainer.kt passes a non-suspend lambda to IMotionVideoProducer.produceVideo(), which expects a suspend function type—this is a compilation error.
MotionVideoContainer.kt (line 120) declares progressListener: ((progress: Int, bitmap: Bitmap) -> Unit)? and passes it to IMotionVideoProducer.produceVideo() (line 136), which now expects (suspend (progress: Int, bitmap: Bitmap) -> Unit)?. Per the Kotlin spec, suspend and non-suspend function types are distinct and not implicitly coercible; this will fail at compile time.
Fix MotionVideoContainer.kt by either:
- Declaring
progressListenerparameter as(suspend (progress: Int, bitmap: Bitmap) -> Unit)?, or - Wrapping the non-suspend lambda to satisfy the suspend type requirement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/IMotionVideoProducer.kt`
around lines 12 - 16, MotionVideoContainer passes a non-suspend lambda to
IMotionVideoProducer.produceVideo which expects a suspend function type; change
the MotionVideoContainer.progressListener declaration to be suspend ((progress:
Int, bitmap: Bitmap) -> Unit)? or wrap the existing non-suspend listener when
calling produceVideo by creating a suspend wrapper (e.g., { p, b ->
existingListener?.invoke(p, b) }) so the passed value matches the suspend
signature expected by IMotionVideoProducer.produceVideo.
| mapOf( | ||
| "startFrame" to startFrame, | ||
| "endFrame" to startFrame, | ||
| ), |
There was a problem hiding this comment.
Bug: endFrame is serialized with the value of startFrame.
Line 12 maps "endFrame" to startFrame instead of endFrame. This silently produces incorrect JSON where both fields have the same value, losing the actual end-frame data.
🐛 Proposed fix
mapOf(
"startFrame" to startFrame,
- "endFrame" to startFrame,
+ "endFrame" to endFrame,
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mapOf( | |
| "startFrame" to startFrame, | |
| "endFrame" to startFrame, | |
| ), | |
| mapOf( | |
| "startFrame" to startFrame, | |
| "endFrame" to endFrame, | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionEffectParser.kt`
around lines 10 - 13, In MotionEffectParser.kt fix the map construction where
"endFrame" is incorrectly assigned startFrame: locate the map (in the method
that serializes effects inside MotionEffectParser) and change the mapping key
"endFrame" to use the endFrame variable instead of startFrame so the serialized
JSON contains the actual end-frame value.
| .toJsonTree( | ||
| mapOf( | ||
| "startFrame" to startFrame, | ||
| "endFrame" to startFrame, |
There was a problem hiding this comment.
endFrame is incorrectly mapped from startFrame — serialized JSON will always have identical values for both fields.
This is a copy-paste bug: "endFrame" to startFrame should read "endFrame" to endFrame. Any consumer that deserializes this JSON (e.g., SduiRenderer / a reverse JsonObject.toMotionView() parser) will receive the wrong end frame, silently breaking animation range logic.
🐛 Proposed fix
- "endFrame" to startFrame,
+ "endFrame" to endFrame,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionViewParser.kt`
at line 12, In MotionViewParser.kt there is a mapping that sets the JSON field
"endFrame" to the variable startFrame (copy-paste bug) so serialized objects
will have identical start and end frames; update the mapping to use the correct
variable endFrame (i.e., change the entry that currently maps "endFrame" to
startFrame so it maps "endFrame" to endFrame) so consumers like SduiRenderer or
JsonObject.toMotionView() receive the correct end-frame value.
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores