Conversation
📝 WalkthroughWalkthroughThis PR introduces three new modular components—datastore, ongoing, and imageloader—that establish Room database persistence, project state management, and Glide-based image loading. It refactors navigation with a new DashboardActivity, updates LyricsViewModel to sync with a ProjectManager, and replaces HttpClient-based image loading with the ImageLoader utility across multiple views. Changes
Sequence Diagram(s)sequenceDiagram
participant DA as DashboardActivity
participant VM as LyricsViewModel
participant PM as ProjectManager
participant DB as ProjectDatabase
participant IL as ImageLoader
participant UI as ProjectCard
DA->>VM: loadDashboardData()
VM->>PM: getAllProjects()
PM->>DB: projectDao.getAll()
DB-->>PM: List<ProjectEntity>
PM-->>VM: List<CurrentProject>
VM-->>DA: dashboardData.emit()
DA->>UI: render ProjectCard(project)
UI->>IL: loadImage(context, project.url)
IL-->>UI: RequestBuilder (async Glide load)
UI->>UI: setImageBitmap(bitmap)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 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
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 @hemusimple, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's functionality by introducing a new dashboard for project management and integrating a robust local storage solution using the Room database. This allows users to save and access their edited song projects persistently, improving the overall user experience and laying the groundwork for future features like album art caching and refined screen routing. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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.
Dashboard Feature Implementation Review
This PR adds a dashboard screen with Room database integration to store project data. While the feature implementation is functional, there are several critical issues that must be addressed before merge:
🚨 Critical Issues (Must Fix)
- Thread Safety: DatabaseProvider singleton implementation has race condition vulnerabilities
- Crash Risk: ProjectManager uses unsafe
lateinitproperties that can cause runtime crashes - Crash Risk: LyricsViewModel's
selectedLyricsgetter will crash on empty lists - Missing Error Handling: Database operations lack proper exception handling
🔧 Required Changes
- Fix double-checked locking pattern in DatabaseProvider
- Replace
lateinitwith nullable properties and add safety checks in ProjectManager - Add empty list validation in LyricsViewModel
- Implement proper error handling for database operations
- Remove debug logging from production code
- Add database migration strategy
📋 Architecture Notes
The overall approach of separating concerns with dedicated modules (datastore, ongoing) is good. The Room integration follows standard patterns, but the implementation needs safety improvements.
Status: ❌ Changes Requested - Please address the critical safety issues before merge.
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.
...atastore/src/main/java/com/tejpratapsingh/motion/datastore/data/database/DatabaseProvider.kt
Show resolved
Hide resolved
| private lateinit var projectDatabase: ProjectDatabase | ||
| private lateinit var projectDao: ProjectDao |
There was a problem hiding this comment.
🛑 Crash Risk: Using lateinit properties without proper initialization checks can cause UninitializedPropertyAccessException at runtime. The saveProject() and getAllProjects() methods can be called before initDataBase(), leading to crashes.
| private lateinit var projectDatabase: ProjectDatabase | |
| private lateinit var projectDao: ProjectDao | |
| private var projectDatabase: ProjectDatabase? = null | |
| private var projectDao: ProjectDao? = null |
| suspend fun saveProject() { | ||
| Log.d("savingData","$songProject") | ||
| projectDao.insert(songProject.toProjectEntity()) | ||
| } | ||
| suspend fun getAllProjects() = projectDao.getAll().map { | ||
| it.toCurrentProject() | ||
| } |
There was a problem hiding this comment.
Add null safety checks to prevent crashes when database is not initialized.
| suspend fun saveProject() { | |
| Log.d("savingData","$songProject") | |
| projectDao.insert(songProject.toProjectEntity()) | |
| } | |
| suspend fun getAllProjects() = projectDao.getAll().map { | |
| it.toCurrentProject() | |
| } | |
| suspend fun saveProject() { | |
| val dao = projectDao ?: throw IllegalStateException("Database not initialized. Call initDataBase() first.") | |
| Log.d("savingData","$songProject") | |
| dao.insert(songProject.toProjectEntity()) | |
| } | |
| suspend fun getAllProjects() = projectDao?.getAll()?.map { | |
| it.toCurrentProject() | |
| } ?: throw IllegalStateException("Database not initialized. Call initDataBase() first.") |
| CoroutineScope(Dispatchers.IO).launch { | ||
| with(ProjectManager) { | ||
| with(songProject) { | ||
| savedFilePath = videoFile.absolutePath | ||
| } | ||
| saveProject() | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for database operations to prevent silent failures when saving project data.
| CoroutineScope(Dispatchers.IO).launch { | |
| with(ProjectManager) { | |
| with(songProject) { | |
| savedFilePath = videoFile.absolutePath | |
| } | |
| saveProject() | |
| } | |
| } | |
| CoroutineScope(Dispatchers.IO).launch { | |
| try { | |
| with(ProjectManager) { | |
| with(songProject) { | |
| savedFilePath = videoFile.absolutePath | |
| } | |
| saveProject() | |
| } | |
| } catch (e: Exception) { | |
| Log.e(TAG, "Failed to save project: ${e.message}", e) | |
| } | |
| } |
| Log.d("getSyncedLyricsWithFrameOffset", "lrcContentString: $lrcContent") | ||
| val parsedResult = parser.parse(lrcContent) |
There was a problem hiding this comment.
Remove debug logging from production code. This Log.d statement will impact performance and potentially expose sensitive lyric content in logs.
| Log.d("getSyncedLyricsWithFrameOffset", "lrcContentString: $lrcContent") | |
| val parsedResult = parser.parse(lrcContent) | |
| val parsedResult = parser.parse(lrcContent) |
| val frame = | ||
| ((it.time / (1000.0 / fps)).toInt() - offsetFrames).coerceAtLeast(0) // avoid negative frames | ||
| Log.d("getSyncedLyricsWithFrameOffset", "(frame,text) - ($frame,${it.text})") | ||
| SyncedLyricFrame( |
There was a problem hiding this comment.
Remove debug logging from production code. This Log.d statement will impact performance and potentially expose sensitive lyric content in logs.
| val frame = | |
| ((it.time / (1000.0 / fps)).toInt() - offsetFrames).coerceAtLeast(0) // avoid negative frames | |
| Log.d("getSyncedLyricsWithFrameOffset", "(frame,text) - ($frame,${it.text})") | |
| SyncedLyricFrame( | |
| val frame = | |
| ((it.time / (1000.0 / fps)).toInt() - offsetFrames).coerceAtLeast(0) // avoid negative frames | |
| SyncedLyricFrame( |
| @Entity(tableName = "projects") | ||
| data class ProjectEntity( | ||
| @PrimaryKey(autoGenerate = true) | ||
| val id: Long = 0L, | ||
| val title: String?, | ||
| val description: String?, | ||
| val image: String?, | ||
| val siteName: String?, | ||
| val twitterCard: String?, | ||
| val url: String?, | ||
| val trackName: String?, | ||
| val artistName: String?, | ||
| val albumName: String?, | ||
| val duration: Float?, | ||
| val instrumental: Boolean?, | ||
| val plainLyrics: String?, | ||
| val syncedLyrics: String?, | ||
| val savedFilePath: String?, | ||
| ) No newline at end of file |
There was a problem hiding this comment.
Add database migration strategy. The database is created without exportSchema = false but no migration handling is implemented. This will cause crashes when the schema changes in future updates.
| @Entity(tableName = "projects") | |
| data class ProjectEntity( | |
| @PrimaryKey(autoGenerate = true) | |
| val id: Long = 0L, | |
| val title: String?, | |
| val description: String?, | |
| val image: String?, | |
| val siteName: String?, | |
| val twitterCard: String?, | |
| val url: String?, | |
| val trackName: String?, | |
| val artistName: String?, | |
| val albumName: String?, | |
| val duration: Float?, | |
| val instrumental: Boolean?, | |
| val plainLyrics: String?, | |
| val syncedLyrics: String?, | |
| val savedFilePath: String?, | |
| ) | |
| @Entity(tableName = "projects") | |
| data class ProjectEntity( | |
| @PrimaryKey(autoGenerate = true) | |
| val id: Long = 0L, | |
| val title: String?, | |
| val description: String?, | |
| val image: String?, | |
| val siteName: String?, | |
| val twitterCard: String?, | |
| val url: String?, | |
| val trackName: String?, | |
| val artistName: String?, | |
| val albumName: String?, | |
| val duration: Float?, | |
| val instrumental: Boolean?, | |
| val plainLyrics: String?, | |
| val syncedLyrics: String?, | |
| val savedFilePath: String?, | |
| val createdAt: Long = System.currentTimeMillis(), // Add timestamp for better data management | |
| ) |
![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 a new dashboard screen and integrates a Room database for persisting project data. The changes are well-structured, with new modules for data and domain logic, which is a great approach for separation of concerns. My review focuses on several key areas: improving performance by reusing network clients, ensuring safer asynchronous operations with coroutines, enhancing the data layer with reactive streams, and removing debug artifacts. I've also included suggestions to follow best practices for data classes and Room configuration.
| val client = HttpClient(CIO) | ||
| client.fetchBitmap(it) |
There was a problem hiding this comment.
Creating a new HttpClient for each image download is highly inefficient. HttpClient instances are heavyweight objects that manage connection pools and thread pools. Creating them repeatedly can lead to significant performance degradation and resource exhaustion. A single HttpClient instance should be created and reused throughout the application. Consider using a singleton object or dependency injection to provide a shared client instance. Also, the created client is not closed, which leads to a resource leak.
| override fun onCompleted(videoFile: File) { | ||
| Log.d(TAG, "onCompleted: Video saved to ${videoFile.absolutePath}") | ||
|
|
||
| CoroutineScope(Dispatchers.IO).launch { |
There was a problem hiding this comment.
Using CoroutineScope(Dispatchers.IO).launch creates a new top-level coroutine that is not bound to any lifecycle. This is a 'fire-and-forget' approach. If saveProject() fails with an exception, it will be unhandled and could crash the application or fail silently. It's safer to use structured concurrency. A simple improvement would be to wrap the operation in a try-catch block to at least log any potential errors.
| @Composable | ||
| fun MetaDataCard(project: CurrentProject,modifier: Modifier = Modifier) { | ||
| var bitmap by remember { mutableStateOf<Bitmap?>(null) } | ||
| LaunchedEffect(project.url) { |
There was a problem hiding this comment.
The LaunchedEffect is keyed by project.url, but the code inside it depends on project.image to load the bitmap. If project.image changes but project.url does not, the effect will not be re-triggered, and the old image will be displayed. The key for LaunchedEffect should be the variable that, when changed, requires the effect to be re-launched. In this case, it should be project.image.
| LaunchedEffect(project.url) { | |
| LaunchedEffect(project.image) { |
| data class ProjectEntity( | ||
| @PrimaryKey(autoGenerate = true) | ||
| val id: Long = 0L, | ||
| val title: String?, | ||
| val description: String?, | ||
| val image: String?, | ||
| val siteName: String?, | ||
| val twitterCard: String?, | ||
| val url: String?, | ||
| val trackName: String?, | ||
| val artistName: String?, | ||
| val albumName: String?, | ||
| val duration: Float?, | ||
| val instrumental: Boolean?, | ||
| val plainLyrics: String?, | ||
| val syncedLyrics: String?, | ||
| val savedFilePath: String?, | ||
| ) No newline at end of file |
There was a problem hiding this comment.
All properties in this data class are nullable. While this provides flexibility, it can also lead to a high number of null checks throughout the codebase and potentially allow for inconsistent or incomplete data to be saved. Consider making fields that should always be present non-nullable and providing default values if appropriate. This will make your data model more robust and your code safer.
| Log.d("getSyncedLyricsWithFrameOffset", "lrcContentString: $lrcContent") | ||
| val parsedResult = parser.parse(lrcContent) | ||
|
|
||
| return parsedResult | ||
| .map { | ||
| val frame = | ||
| ((it.time / (1000.0 / fps)).toInt() - offsetFrames).coerceAtLeast(0) // avoid negative frames | ||
| Log.d("getSyncedLyricsWithFrameOffset", "(frame,text) - ($frame,${it.text})") |
There was a problem hiding this comment.
There are debug Log.d calls present in this file. These should be removed before merging to a production branch to keep the logs clean and avoid any minor performance impact.
| _dashboardData.value = projectManager.getAllProjects().also { | ||
| Log.d("savedData","${it}") | ||
| } |
There was a problem hiding this comment.
This function contains a debug logging statement. It should be removed before merging to a production branch.
_dashboardData.value = projectManager.getAllProjects()| suspend fun insert(projectEntity: ProjectEntity) | ||
|
|
||
| @Query("SELECT * FROM projects") | ||
| suspend fun getAll(): List<ProjectEntity> |
There was a problem hiding this comment.
For better integration with Compose and to build a reactive UI, consider returning a Flow<List<ProjectEntity>> from this function instead of a List<ProjectEntity>. This will allow your UI to automatically observe changes in the database and update itself without manual data reloading.
| suspend fun getAll(): List<ProjectEntity> | |
| suspend fun getAll(): Flow<List<ProjectEntity>> |
| import com.tejpratapsingh.motion.datastore.data.dao.ProjectDao | ||
| import com.tejpratapsingh.motion.datastore.data.entity.ProjectEntity | ||
|
|
||
| @Database(entities = [ProjectEntity::class], version = 1, exportSchema = false) |
There was a problem hiding this comment.
It's a best practice to enable schema exporting for Room databases, especially in production apps. Setting exportSchema to true allows you to version control your database schema, which is invaluable for tracking history and managing migrations. You'll need to specify the schema location in your module's build.gradle file.
| @Database(entities = [ProjectEntity::class], version = 1, exportSchema = false) | |
| @Database(entities = [ProjectEntity::class], version = 1, exportSchema = true) |
| data class CurrentProject( | ||
| var title: String? = null, | ||
| var description: String? = null, | ||
| var image: String? = null, | ||
| var siteName: String? = null, | ||
| var twitterCard: String? = null, | ||
| var url: String? = null, | ||
| var trackName: String? = null, | ||
| var artistName: String? = null, | ||
| var albumName: String? = null, | ||
| var duration: Float? = null, | ||
| var instrumental: Boolean? = null, | ||
| var plainLyrics: String? = null, | ||
| var syncedLyrics: String? = null, | ||
| var savedFilePath: String?=null, | ||
| ) No newline at end of file |
There was a problem hiding this comment.
This data class uses var for its properties, making it mutable. While convenient, this can lead to unpredictable behavior and bugs, as the object's state can be modified from anywhere. It's a best practice to favor immutability by using val for properties. When you need to change the state, you can create a new instance using the copy() method provided for data classes. This makes state management safer and more predictable.
| projectDao = projectDatabase.projectDao() | ||
| } | ||
| suspend fun saveProject() { | ||
| Log.d("savingData","$songProject") |
There was a problem hiding this comment.
This function contains a debug logging statement. It should be removed from production code.
| Log.d("savingData","$songProject") | |
| Log.d("savingData", "$songProject") |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (9)
modules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/dao/ProjectDao.kt (2)
15-16: Consider pagination for project list retrieval.The
getAll()method loads all projects into memory at once. As the number of saved projects grows, this could impact performance and memory usage.🔎 Suggested alternatives
Option 1: Add a paginated query using Room's PagingSource
@Query("SELECT * FROM projects ORDER BY id DESC") fun getAllPaged(): PagingSource<Int, ProjectEntity>Option 2: Add a limit parameter for basic pagination
@Query("SELECT * FROM projects ORDER BY id DESC LIMIT :limit OFFSET :offset") suspend fun getAllPaged(limit: Int, offset: Int): List<ProjectEntity>
9-17: Consider adding common CRUD operations.The DAO currently only supports insert and getAll. Consider adding update, delete, and getById operations for more complete data access functionality.
🔎 Suggested additions
@Query("SELECT * FROM projects WHERE id = :id") suspend fun getById(id: Long): ProjectEntity? @Update suspend fun update(projectEntity: ProjectEntity) @Delete suspend fun delete(projectEntity: ProjectEntity) @Query("DELETE FROM projects WHERE id = :id") suspend fun deleteById(id: Long)modules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/database/ProjectDatabase.kt (1)
8-8: Consider enabling schema export for version control.Setting
exportSchema = falseprevents Room from exporting the database schema to a JSON file. Exported schemas help track schema changes over time and are useful for writing and testing migrations.🔎 Recommended change
In your module's
build.gradle:android { defaultConfig { javaCompileOptions { annotationProcessorOptions { arguments += ["room.schemaLocation": "$projectDir/schemas".toString()] } } } }Then update the annotation:
-@Database(entities = [ProjectEntity::class], version = 1, exportSchema = false) +@Database(entities = [ProjectEntity::class], version = 1, exportSchema = true) abstract class ProjectDatabase : RoomDatabase() { abstract fun projectDao(): ProjectDao }Don't forget to add the
schemas/directory to version control.modules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/entity/ProjectEntity.kt (1)
6-24: LGTM!The entity structure is correct with proper Room annotations, auto-generated primary key, and immutable fields.
Consider adding indices if queries will filter or sort by specific fields (e.g.,
trackName,artistName,savedFilePath). For example:@Entity( tableName = "projects", indices = [Index(value = ["trackName"]), Index(value = ["artistName"])] )modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/DashboardActivity.kt (1)
15-15: Potentially unnecessary import.The
kotlin.getValueimport on Line 15 may be redundant, as theby viewModels()delegate typically works without explicitly importing it. Consider removing this import unless it's required by your specific Kotlin/IDE configuration.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/DashBoardScreen.kt (2)
39-54: Consider adding loading and error states.The
LaunchedEffect(Unit)triggers data loading on every composition, but there's no visual feedback for loading or error states. Users won't know if data is being fetched or if loading failed.🔎 Suggested enhancement for user feedback
Consider exposing loading/error states from the ViewModel and displaying appropriate UI:
@Composable fun DashBoardScreen(lyricsViewModel: LyricsViewModel) { val projects by lyricsViewModel.dashboardData.collectAsState() val isLoading by lyricsViewModel.isLoadingDashboard.collectAsState() val error by lyricsViewModel.dashboardError.collectAsState() LaunchedEffect(Unit) { lyricsViewModel.loadDashboardData() } when { isLoading -> CircularProgressIndicator(modifier = Modifier.fillMaxSize()) error != null -> ErrorView(error = error) else -> LazyVerticalGrid( columns = GridCells.Fixed(2), contentPadding = PaddingValues(8.dp) ) { items(projects) { item -> MetaDataCard(item, modifier = Modifier.padding(8.dp)) } } } }
57-63: Image loading needs caching (as noted in PR description).Each
MetaDataCardloads its bitmap independently viaproject.getLoadBitmap(), likely making HTTP requests without caching. As you mentioned in the PR description, this should be addressed to avoid repeated network calls.Consider implementing one of these caching strategies:
- Use a library like Coil or Glide that handles caching automatically
- Implement a shared image cache in the ViewModel or a repository layer
- Use Room to cache downloaded images locally
Would you like me to provide a sample implementation using Coil or another approach?
modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/ProjectManager.kt (1)
11-28: Consider dependency injection instead of singleton pattern.The singleton pattern with mutable state makes testing difficult and creates tight coupling. Consider refactoring to use dependency injection (e.g., Hilt/Koin) with a Repository pattern for better testability and lifecycle management.
Benefits:
- Easier to mock in unit tests
- Better scope management (e.g., scoped to Activity/ViewModel)
- Follows Android architecture best practices
- Eliminates thread-safety concerns with proper scoping
Example Repository approach:
class ProjectRepository(private val projectDao: ProjectDao) { var currentProject = CurrentProject() private set suspend fun saveProject(): Result<Unit> = runCatching { projectDao.insert(currentProject.toProjectEntity()) } suspend fun getAllProjects(): Result<List<CurrentProject>> = runCatching { projectDao.getAll().map { it.toCurrentProject() } } }Then inject via Hilt/Koin into ViewModels.
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt (1)
20-20: Consider injecting ProjectManager for better testability.The default parameter
ProjectManager = ProjectManagertightly couples this ViewModel to the singleton. While it allows overriding for tests, constructor-based dependency injection (via Hilt/Koin) would be more maintainable and follow Android best practices.Example with Hilt:
@HiltViewModel class LyricsViewModel @Inject constructor( private val projectManager: ProjectManager ) : ViewModel()This also aligns with the suggestion to refactor
ProjectManagerfrom a singleton to an injected dependency (see comments on ProjectManager.kt).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
gradle/libs.versions.toml(3 hunks)modules/datastore/.gitignore(1 hunks)modules/datastore/build.gradle(1 hunks)modules/datastore/proguard-rules.pro(1 hunks)modules/datastore/src/main/AndroidManifest.xml(1 hunks)modules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/dao/ProjectDao.kt(1 hunks)modules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/database/DatabaseProvider.kt(1 hunks)modules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/database/ProjectDatabase.kt(1 hunks)modules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/entity/ProjectEntity.kt(1 hunks)modules/lyrics-maker/build.gradle(1 hunks)modules/lyrics-maker/src/main/AndroidManifest.xml(3 hunks)modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/LyricMaker.kt(1 hunks)modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/lrc/LrcHelper.kt(2 hunks)modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/DashboardActivity.kt(1 hunks)modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt(2 hunks)modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/AppNavHost.kt(2 hunks)modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/DashBoardScreen.kt(1 hunks)modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt(3 hunks)modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.kt(2 hunks)modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/utils/ProjectImageHelper.kt(1 hunks)modules/ongoing/.gitignore(1 hunks)modules/ongoing/build.gradle(1 hunks)modules/ongoing/proguard-rules.pro(1 hunks)modules/ongoing/src/main/AndroidManifest.xml(1 hunks)modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/CurrentProject.kt(1 hunks)modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/ProjectManager.kt(1 hunks)modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/mapper/Mapper.kt(1 hunks)settings.gradle(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/AppNavHost.kt (1)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/DashBoardScreen.kt (1)
DashBoardScreen(38-54)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.kt (1)
modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/ProjectManager.kt (1)
saveProject(21-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Amazon Q Developer
- GitHub Check: Build lyrics-maker
- GitHub Check: Build app
🔇 Additional comments (19)
modules/datastore/src/main/AndroidManifest.xml (1)
1-2: LGTM!The minimal manifest is appropriate for a library module providing Room database persistence. No runtime components or permissions are needed for local database operations.
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/lrc/LrcHelper.kt (1)
3-3: LGTM: Import is necessary for the debug statements.The Log import is required for the debug logging added below.
modules/datastore/.gitignore (1)
1-1: LGTM!Standard .gitignore configuration for Android module build outputs.
modules/ongoing/src/main/AndroidManifest.xml (1)
1-2: LGTM!Empty manifest is appropriate for an Android library module with no declared components.
modules/ongoing/.gitignore (1)
1-1: LGTM!Standard .gitignore configuration for Android module build outputs.
modules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/dao/ProjectDao.kt (1)
12-13: Verify the OnConflictStrategy.REPLACE behavior.The REPLACE strategy will silently overwrite existing projects with the same primary key. Ensure this is the intended behavior, as it may lead to unexpected data loss if a project is accidentally re-inserted.
Consider using OnConflictStrategy.ABORT or IGNORE if you want to preserve existing data or handle conflicts explicitly.
modules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/database/DatabaseProvider.kt (1)
6-21: LGTM on the singleton pattern implementation!The double-checked locking with
@Volatileis correctly implemented for thread-safe singleton initialization, and usingapplicationContextprevents memory leaks.modules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/database/ProjectDatabase.kt (1)
8-11: LGTM on the database structure!The Room database is correctly configured with proper annotations and accessor methods.
modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/mapper/Mapper.kt (1)
6-21: LGTM!The mapping from
CurrentProjecttoProjectEntityis correct. Theidfield defaults to0L, which is appropriate for Room's auto-generation.modules/lyrics-maker/build.gradle (1)
62-62: LGTM!The dependency addition correctly integrates the ongoing module for project management functionality.
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt (1)
54-54: Explicit navigation start destination set to Home screen.The start destination is explicitly set to
Screen.Home.route, which overrides the defaultScreen.Dashboard.routementioned in the AI summary. This ensuresSearchActivitystarts at the Home screen as intended for handling shared content.settings.gradle (1)
34-35: LGTM!The module inclusions correctly register the new
datastoreandongoingmodules in the build configuration.gradle/libs.versions.toml (1)
32-33: Room and KSP versions are current and compatible.Room version 2.8.4 is the latest stable release, and KSP version 2.2.21-2.0.4 is designed to support Kotlin 2.2.21. No updates needed.
modules/ongoing/build.gradle (1)
1-47: LGTM!The build configuration is appropriate for the ongoing module, with proper Android library setup, KSP for annotation processing, and the correct dependency on the datastore module.
modules/datastore/build.gradle (1)
1-50: LGTM!The build configuration correctly sets up the datastore module with Room dependencies using
apivisibility, which is appropriate since consuming modules need access to Room types and DAOs.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/DashboardActivity.kt (1)
17-37: LGTM!The activity implementation correctly follows Compose best practices with proper ViewModel scoping, edge-to-edge support, and clean integration with the navigation host.
modules/lyrics-maker/src/main/AndroidManifest.xml (1)
10-10: LGTM!The manifest changes correctly wire up the new
LyricMakerapplication class for database initialization and designateDashboardActivityas the new launcher activity, while preservingSearchActivityaccessibility via the DEFAULT category.Also applies to: 25-25, 34-43
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/AppNavHost.kt (1)
19-19: LGTM!The navigation changes cleanly integrate the Dashboard screen as the default destination while maintaining the existing Home and Lyrics routes. The implementation follows Jetpack Compose navigation best practices.
Also applies to: 26-26, 30-30, 42-46
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/DashBoardScreen.kt (1)
65-109: LGTM!The card layout is well-designed with appropriate gradient overlay for text readability and proper null safety handling for optional fields.
| # Add project specific ProGuard rules here. | ||
| # You can control the set of applied configuration files using the | ||
| # proguardFiles setting in build.gradle. | ||
| # | ||
| # For more details, see | ||
| # http://developer.android.com/guide/developing/tools/proguard.html | ||
|
|
||
| # If your project uses WebView with JS, uncomment the following | ||
| # and specify the fully qualified class name to the JavaScript interface | ||
| # class: | ||
| #-keepclassmembers class fqcn.of.javascript.interface.for.webview { | ||
| # public *; | ||
| #} | ||
|
|
||
| # Uncomment this to preserve the line number information for | ||
| # debugging stack traces. | ||
| #-keepattributes SourceFile,LineNumberTable | ||
|
|
||
| # If you keep the line number information, uncomment this to | ||
| # hide the original source file name. | ||
| #-renamesourcefileattribute SourceFile No newline at end of file |
There was a problem hiding this comment.
Add Room-specific ProGuard rules.
The datastore module contains Room entities and DAOs that must be preserved during code shrinking and obfuscation. Without proper ProGuard/R8 rules, release builds will fail at runtime with reflection errors.
🔎 Recommended ProGuard rules for Room
# Add project specific ProGuard rules here.
# You can control the set of applied configuration files using the
# proguardFiles setting in build.gradle.
#
# For more details, see
# http://developer.android.com/guide/developing/tools/proguard.html
+# Room-specific rules
+-keep class * extends androidx.room.RoomDatabase
+-keep @androidx.room.Entity class *
+-dontwarn androidx.room.paging.**
+
# If your project uses WebView with JS, uncomment the following
# and specify the fully qualified class name to the JavaScript interface
# class:
#-keepclassmembers class fqcn.of.javascript.interface.for.webview {
# public *;
#}
# Uncomment this to preserve the line number information for
# debugging stack traces.
#-keepattributes SourceFile,LineNumberTable
# If you keep the line number information, uncomment this to
# hide the original source file name.
#-renamesourcefileattribute SourceFile🤖 Prompt for AI Agents
In modules/datastore/proguard-rules.pro around lines 1 to 21, add ProGuard/R8
rules that preserve Room entities, DAOs and generated database classes so they
aren’t removed/obfuscated at build time: keep all classes annotated with
androidx.room.Entity, androidx.room.Dao and androidx.room.Database (and their
members used via reflection), keep Room-generated implementation classes (Room_*
and any Impl classes), retain Room annotations and metadata (kotlin.Metadata if
using Kotlin), and preserve any constructors or fields accessed reflectively by
Room; add these entries to this file so release builds won’t fail at runtime due
to missing Room classes.
| fun getDatabase(context: Context): ProjectDatabase { | ||
| return INSTANCE ?: synchronized(this) { | ||
| val instance = Room.databaseBuilder( | ||
| context.applicationContext, | ||
| ProjectDatabase::class.java, | ||
| "LyricMaker" | ||
| ).build() | ||
| INSTANCE = instance | ||
| instance | ||
| } | ||
| } |
There was a problem hiding this comment.
Add database migration strategy.
The database builder doesn't specify a migration strategy. When you update the schema in future versions (e.g., adding columns, new tables), the app will crash unless you provide migrations or explicitly allow destructive migrations.
🔎 Recommended approach
For development/MVP, add fallback to destructive migration:
fun getDatabase(context: Context): ProjectDatabase {
return INSTANCE ?: synchronized(this) {
val instance = Room.databaseBuilder(
context.applicationContext,
ProjectDatabase::class.java,
"LyricMaker"
- ).build()
+ )
+ .fallbackToDestructiveMigration()
+ .build()
INSTANCE = instance
instance
}
}Note: For production, replace with proper migrations to preserve user data:
.addMigrations(MIGRATION_1_2, MIGRATION_2_3)📝 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.
| fun getDatabase(context: Context): ProjectDatabase { | |
| return INSTANCE ?: synchronized(this) { | |
| val instance = Room.databaseBuilder( | |
| context.applicationContext, | |
| ProjectDatabase::class.java, | |
| "LyricMaker" | |
| ).build() | |
| INSTANCE = instance | |
| instance | |
| } | |
| } | |
| fun getDatabase(context: Context): ProjectDatabase { | |
| return INSTANCE ?: synchronized(this) { | |
| val instance = Room.databaseBuilder( | |
| context.applicationContext, | |
| ProjectDatabase::class.java, | |
| "LyricMaker" | |
| ) | |
| .fallbackToDestructiveMigration() | |
| .build() | |
| INSTANCE = instance | |
| instance | |
| } | |
| } |
🤖 Prompt for AI Agents
In
modules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/database/DatabaseProvider.kt
around lines 10 to 20, the Room database is built without any migration strategy
which will crash the app on schema changes; update the builder to either
register proper migrations with .addMigrations(...) for production or, for
development/MVP, add .fallbackToDestructiveMigration() to avoid crashes, and
ensure you declare any Migration objects (e.g., MIGRATION_1_2) and pass them
into .addMigrations(...) when opting for non-destructive updates.
| offsetFrames: Int = 0, | ||
| parser: LrcParser = LrcParser(), | ||
| ): List<SyncedLyricFrame> { | ||
| Log.d("getSyncedLyricsWithFrameOffset", "lrcContentString: $lrcContent") |
There was a problem hiding this comment.
Remove debug logging of lyric content before merging.
Logging the entire lrcContent string raises compliance and privacy concerns:
- Lyric text may be copyrighted material
- Could contain user-generated content or PII
- May produce large log output
Debug logs like this should be removed before the PR is merged to avoid leaking sensitive or copyrighted data.
🔎 Proposed fix
- Log.d("getSyncedLyricsWithFrameOffset", "lrcContentString: $lrcContent")📝 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.
| Log.d("getSyncedLyricsWithFrameOffset", "lrcContentString: $lrcContent") |
🤖 Prompt for AI Agents
In
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/lrc/LrcHelper.kt
around line 23, remove the debug Log.d("getSyncedLyricsWithFrameOffset",
"lrcContentString: $lrcContent") that logs the full lyric content; instead
either delete the log statement entirely or replace it with a non-sensitive
message (e.g., log only the length or a boolean indicating content presence) to
avoid exposing copyrighted or PII data.
| .map { | ||
| val frame = | ||
| ((it.time / (1000.0 / fps)).toInt() - offsetFrames).coerceAtLeast(0) // avoid negative frames | ||
| Log.d("getSyncedLyricsWithFrameOffset", "(frame,text) - ($frame,${it.text})") |
There was a problem hiding this comment.
Remove debug logging inside the map operation before merging.
This log statement has multiple concerns:
- Logs lyric text (copyrighted or user content), creating the same privacy/compliance risks as Line 23
- Executes inside a loop for every lyric line, potentially generating excessive log output
- May impact performance when processing large lyric files
Remove this debug statement before merging.
🔎 Proposed fix
- Log.d("getSyncedLyricsWithFrameOffset", "(frame,text) - ($frame,${it.text})")📝 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.
| Log.d("getSyncedLyricsWithFrameOffset", "(frame,text) - ($frame,${it.text})") |
🤖 Prompt for AI Agents
In
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/lrc/LrcHelper.kt
around line 30, remove the in-loop debug Log.d("getSyncedLyricsWithFrameOffset",
"(frame,text) - ($frame,${it.text})") from inside the map operation to avoid
logging user/copyrighted lyric text and excessive output/performance impact;
simply delete the Log.d call (or guard it behind a non-production flag if
absolutely needed) so the map only transforms data without side-effect logging.
| class LyricMaker : Application() { | ||
| override fun onCreate() { | ||
| super.onCreate() | ||
| ProjectManager.initDataBase(applicationContext) | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find ProjectManager class and initDataBase method
find . -type f -name "*.kt" -o -name "*.java" | head -20
rg "class ProjectManager|object ProjectManager" -t kt -t javaRepository: tejpratap46/AndroidVideoMotion
Length of output: 2006
🏁 Script executed:
# Search for ProjectManager class definition
rg "ProjectManager" --type kotlin --type java -A 5
# Also try to find the file
find . -name "*ProjectManager*"Repository: tejpratap46/AndroidVideoMotion
Length of output: 5496
🏁 Script executed:
# Read the ProjectManager.kt file to see the initDataBase implementation
cat modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/ProjectManager.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 1121
🏁 Script executed:
# Find and read the DatabaseProvider implementation
rg "class DatabaseProvider|object DatabaseProvider" --type kotlin --type java -A 20Repository: tejpratap46/AndroidVideoMotion
Length of output: 2161
Move database initialization off the main thread or handle potential blocking operations.
ProjectManager.initDataBase(applicationContext) calls Room.databaseBuilder().build(), which is a synchronous operation that blocks the main thread. While database initialization is typically fast, this can still delay app startup. Additionally, there's no error handling if initialization fails.
Consider running initialization on a background thread or using lazy initialization:
lifecycleScope.launchWhenCreated {
withContext(Dispatchers.IO) {
ProjectManager.initDataBase(applicationContext)
}
}Also wrap with try-catch to handle potential initialization failures gracefully.
🤖 Prompt for AI Agents
In
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/LyricMaker.kt
around lines 6 to 11, ProjectManager.initDataBase(applicationContext) is called
on the main thread and blocks because Room.databaseBuilder().build() is
synchronous; move the initialization off the main thread and add error handling:
call initDataBase from a background context (e.g., Dispatchers.IO via a
CoroutineScope tied to Application or an executor) or switch to lazy
initialization so DB is built on first access, and wrap the call in try-catch to
log or recover from initialization failures gracefully.
| fun ProjectEntity.toCurrentProject() = CurrentProject( | ||
| title = this.title, | ||
| description = this.description, | ||
| image = this.image, | ||
| siteName = this.siteName, | ||
| twitterCard = this.twitterCard, | ||
| url = this.url, | ||
| trackName = this.trackName, | ||
| artistName = this.artistName, | ||
| albumName = this.albumName, | ||
| duration = this.duration, | ||
| instrumental = this.instrumental, | ||
| plainLyrics = this.plainLyrics, | ||
| syncedLyrics = this.syncedLyrics, | ||
| savedFilePath = this.savedFilePath, | ||
| ) No newline at end of file |
There was a problem hiding this comment.
Missing trailing newline.
The mapping logic is correct, but the file should end with a newline for consistency with Kotlin conventions.
🤖 Prompt for AI Agents
In
modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/mapper/Mapper.kt
around lines 23 to 38, the file is missing a trailing newline at the end; add a
single newline character at EOF so the file ends with a newline (ensure the last
line break is present after the closing parenthesis/comma) and save the file to
conform to Kotlin/Unix newline conventions.
| object ProjectManager { | ||
|
|
||
| private lateinit var projectDatabase: ProjectDatabase | ||
| private lateinit var projectDao: ProjectDao | ||
| var songProject = CurrentProject() | ||
| private set |
There was a problem hiding this comment.
Address thread-safety concerns in singleton with mutable state.
This singleton object contains mutable state (songProject) and lateinit properties that can be accessed concurrently from multiple threads without synchronization. This can lead to race conditions and visibility issues.
Consider one of these approaches:
- Add
@Synchronizedannotations to methods that access shared state - Use thread-safe data structures or atomic references
- Refactor to use a Repository pattern with proper scope management instead of a singleton
🔎 Quick fix with synchronization
var songProject = CurrentProject()
private set
- fun initDataBase(context: Context) {
+ @Synchronized
+ fun initDataBase(context: Context) {
projectDatabase = DatabaseProvider.getDatabase(context)
projectDao = projectDatabase.projectDao()
}
+ @Synchronized
suspend fun saveProject() {
Log.d("savingData","$songProject")
projectDao.insert(songProject.toProjectEntity())
}
+ @Synchronized
suspend fun getAllProjects() = projectDao.getAll().map {
it.toCurrentProject()
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/ProjectManager.kt
around lines 11 to 16, the singleton exposes mutable state (songProject) and
lateinit DAO/DB fields that are not thread-safe; change these to be safely
initialized and accessed by either making initialization and all accessors
synchronized or by switching songProject to an AtomicReference<CurrentProject>
and wrapping projectDatabase/projectDao in a thread-safe lazy initializer (or
synchronized init method). Specifically, replace direct mutable access with
synchronized getter/setter methods (or @Synchronized annotations) for
songProject and ensure projectDatabase/projectDao are initialized with a
synchronized/volatile check-or-use Kotlin's synchronized lazy delegate so
concurrent threads cannot observe partially-initialized state. Ensure all public
methods that read or mutate these fields call the synchronized accessors.
| private lateinit var projectDatabase: ProjectDatabase | ||
| private lateinit var projectDao: ProjectDao |
There was a problem hiding this comment.
Add initialization checks to prevent crashes.
The lateinit properties can cause UninitializedPropertyAccessException if initDataBase is not called before saveProject or getAllProjects. Consider adding initialization checks or using nullable types with proper null handling.
🔎 Proposed fix with initialization check
+ private fun ensureInitialized() {
+ if (!::projectDatabase.isInitialized || !::projectDao.isInitialized) {
+ throw IllegalStateException("ProjectManager must be initialized via initDataBase() before use")
+ }
+ }
+
suspend fun saveProject() {
+ ensureInitialized()
Log.d("savingData","$songProject")
projectDao.insert(songProject.toProjectEntity())
}
suspend fun getAllProjects() = projectDao.getAll().map {
+ ensureInitialized()
it.toCurrentProject()
}Alternatively, consider using nullable types:
- private lateinit var projectDatabase: ProjectDatabase
- private lateinit var projectDao: ProjectDao
+ private var projectDatabase: ProjectDatabase? = null
+ private var projectDao: ProjectDao? = nullCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/ProjectManager.kt
around lines 13-14, the lateinit properties projectDatabase and projectDao can
throw UninitializedPropertyAccessException if initDataBase isn't called; add
guard checks before any use (e.g., validate ::projectDatabase.isInitialized and
::projectDao.isInitialized and throw a clear IllegalStateException or call init
logic), or change them to nullable types and handle nulls safely in
saveProject/getAllProjects (return empty list or propagate a clear error).
Ensure every public method that uses these fields either performs the check and
a meaningful error or lazily initializes them so the class cannot crash when
initDataBase was missed.
| suspend fun saveProject() { | ||
| Log.d("savingData","$songProject") | ||
| projectDao.insert(songProject.toProjectEntity()) | ||
| } | ||
| suspend fun getAllProjects() = projectDao.getAll().map { | ||
| it.toCurrentProject() | ||
| } |
There was a problem hiding this comment.
Add error handling for database operations.
The suspend functions saveProject and getAllProjects perform database operations without error handling. Database failures (e.g., constraint violations, disk full, corruption) will propagate as uncaught exceptions.
🔎 Proposed fix with error handling
- suspend fun saveProject() {
+ suspend fun saveProject(): Result<Unit> = runCatching {
Log.d("savingData","$songProject")
projectDao.insert(songProject.toProjectEntity())
}
- suspend fun getAllProjects() = projectDao.getAll().map {
+
+ suspend fun getAllProjects(): Result<List<CurrentProject>> = runCatching {
+ projectDao.getAll().map {
- it.toCurrentProject()
+ it.toCurrentProject()
+ }
}🤖 Prompt for AI Agents
In
modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/ProjectManager.kt
around lines 21 to 27, the suspend functions saveProject and getAllProjects
perform DAO operations without error handling; wrap the insert and getAll calls
in try/catch blocks, log the exception with context (e.g., using Log.e or a
logger), and either rethrow a wrapped exception or return a safe fallback (for
getAllProjects return an empty list or Result type) so failures don't propagate
uncaught; ensure coroutine exceptions are handled and consider adding specific
handling for common DB errors (constraint/disk) and unit tests for the error
paths.
| projectDao = projectDatabase.projectDao() | ||
| } | ||
| suspend fun saveProject() { | ||
| Log.d("savingData","$songProject") |
There was a problem hiding this comment.
Avoid logging potentially sensitive user data.
Logging the entire songProject object may expose PII such as song titles, artist names, descriptions, URLs, and album art paths. This could violate privacy regulations (GDPR/CCPA) if logs are persisted or transmitted.
Consider logging only non-sensitive metadata (e.g., project ID, operation status) or removing the log statement in production builds:
- Log.d("savingData","$songProject")
+ Log.d("savingData","Saving project with ${projectDao.getAll().size} total projects")Or use a conditional debug flag:
- Log.d("savingData","$songProject")
+ if (BuildConfig.DEBUG) {
+ Log.d("savingData","$songProject")
+ }📝 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.
| Log.d("savingData","$songProject") | |
| if (BuildConfig.DEBUG) { | |
| Log.d("savingData","$songProject") | |
| } |
🤖 Prompt for AI Agents
In
modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/ProjectManager.kt
around line 22, remove or replace the Log.d("savingData","$songProject") call
that logs the full songProject (which can contain PII). Instead log only
non-sensitive identifiers and status (e.g., project ID and operation outcome) or
guard the log with a debug/build flag so it is never emitted in production;
ensure no titles, artist names, URLs, descriptions, or file paths are included
in the logged output.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
modules/datastore/src/test/java/com/tejpratapsingh/motion/datastore/ExampleUnitTest.kt (1)
11-16: Consider adding meaningful tests for the datastore module.This placeholder test doesn't validate any of the newly introduced datastore functionality (ProjectEntity, ProjectDao, ProjectDatabase, or DatabaseProvider mentioned in the PR). Consider adding tests that verify database operations, DAO queries, and entity mapping.
modules/ongoing/src/test/java/com/tejpratapsingh/motion/ongoing/ExampleUnitTest.kt (1)
11-16: Consider adding meaningful tests for the ongoing module.This placeholder test doesn't validate any of the domain logic introduced in the ongoing module (CurrentProject, ProjectManager, or Mapper classes mentioned in the PR). Consider adding tests that verify project management operations and data mapping logic.
Additionally, this exact placeholder pattern is duplicated across multiple modules in the codebase (motionlib, jcodec-motion-ext, core, tensorflow-motion-ext, ffmpeg-motion-ext, pytorch-motion-ext, templates, sdui, and metadata-extractor), which could be addressed in a future cleanup effort.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
gradle/libs.versions.toml(3 hunks)modules/datastore/src/test/java/com/tejpratapsingh/motion/datastore/ExampleUnitTest.kt(1 hunks)modules/lyrics-maker/build.gradle(1 hunks)modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt(4 hunks)modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/DashBoardScreen.kt(1 hunks)modules/ongoing/src/test/java/com/tejpratapsingh/motion/ongoing/ExampleUnitTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- modules/lyrics-maker/build.gradle
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/DashBoardScreen.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt
🧰 Additional context used
🧬 Code graph analysis (1)
modules/datastore/src/test/java/com/tejpratapsingh/motion/datastore/ExampleUnitTest.kt (9)
modules/metadata-extractor/src/test/java/com/tejpratapsingh/motion/metadataextractor/ExampleUnitTest.kt (2)
addition_isCorrect(11-16)addition_isCorrect(12-15)modules/motionlib/src/test/java/com/tejpratapsingh/motionlib/ExampleUnitTest.kt (1)
addition_isCorrect(11-16)modules/jcodec-motion-ext/src/test/java/com/tejpratapsingh/motionlib/jcodec/ExampleUnitTest.kt (1)
addition_isCorrect(11-16)modules/core/src/test/java/com/tejpratapsingh/motionlib/core/ExampleUnitTest.kt (1)
addition_isCorrect(11-16)modules/tensorflow-motion-ext/src/test/java/com/tejpratapsingh/motionlib/tensorflow/ExampleUnitTest.kt (1)
addition_isCorrect(11-16)modules/ffmpeg-motion-ext/src/test/java/com/tejpratapsingh/motionlib/ffmpeg/ExampleUnitTest.kt (1)
addition_isCorrect(11-16)modules/pytorch-motion-ext/src/test/java/com/tejpratapsingh/motionlib/pytorch/ExampleUnitTest.kt (1)
addition_isCorrect(11-16)modules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/ExampleUnitTest.kt (1)
addition_isCorrect(11-16)modules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/ExampleUnitTest.kt (1)
addition_isCorrect(11-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build app
- GitHub Check: Build lyrics-maker
🔇 Additional comments (4)
gradle/libs.versions.toml (4)
80-80: LGTM! Version managed by Compose BOM.The
material-icons-extendedlibrary correctly omits an explicit version reference, as it's managed by the Compose BOM (defined at line 72), consistent with other Compose UI libraries in this catalog.
84-88: LGTM! Room library declarations are correctly structured.The Room library entries follow proper version catalog conventions and include all essential components (runtime, compiler, ktx, testing). The declarations are well-organized and reference the
room_versionconsistently.
98-98: LGTM! KSP plugin declaration is correctly structured.The KSP plugin entry follows proper version catalog conventions with the correct plugin ID and version reference.
32-33: The versions specified are valid and compatible. Room 2.8.4 is the current recommended version, and KSP 2.2.21-2.0.4 correctly matches the Kotlin 2.2.21 compiler.
| @@ -0,0 +1,16 @@ | |||
| package com.tejpratapsingh.motion.metadataextractor | |||
There was a problem hiding this comment.
Fix the package name to match the module structure.
The package declaration is com.tejpratapsingh.motion.metadataextractor, but the file is located in the datastore module. This mismatch will cause compilation errors and incorrect imports.
🔎 Proposed fix
-package com.tejpratapsingh.motion.metadataextractor
+package com.tejpratapsingh.motion.datastore📝 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.
| package com.tejpratapsingh.motion.metadataextractor | |
| package com.tejpratapsingh.motion.datastore |
🤖 Prompt for AI Agents
In
modules/datastore/src/test/java/com/tejpratapsingh/motion/datastore/ExampleUnitTest.kt
around line 1, the file has package com.tejpratapsingh.motion.metadataextractor
which does not match the datastore module path; change the package declaration
to the datastore package (e.g., com.tejpratapsingh.motion.datastore) so it
matches the directory structure, and update any imports or references in the
test to use the corrected package.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt:
- Line 46: The declared SeekBar field 'progress' (and other SeekBar instances in
this class) is interactive by default but has no listener or disablement; decide
whether it should be read-only or allow seeking: if read-only call
setEnabled(false) / setOnTouchListener { _, _ -> true } on 'progress' (and the
other SeekBars) to prevent user interaction, otherwise attach
setOnSeekBarChangeListener(...) to 'progress' and implement
onStartTrackingTouch/onStopTrackingTouch/onProgressChanged to update the
playback/animation position and sync UI state.
In @modules/lyrics-maker/src/main/res/layout/lyrics_container.xml:
- Around line 111-139: The layout defines ImageButtons btn_previous,
btn_play_pause and btn_next but they are not wired to actions; open
LyricsContainer.kt (or the hosting Activity/Fragment), find where the view is
inflated/initialized and call findViewById (or view binding) to get references
to btn_previous, btn_play_pause and btn_next and attach setOnClickListener
handlers for each; inside those listeners invoke the appropriate methods or
callbacks (e.g., onPrevious(), togglePlayPause()/onPlayPause(), onNext() or a
provided MediaControlCallback) to perform the media actions and update UI state
(play/pause icon) as needed.
🧹 Nitpick comments (2)
.idea/deploymentTargetSelector.xml (1)
10-17: Consider excluding IDE deployment target configuration from version control.This file captures developer-specific device selections and contains a hardcoded device identifier (IP
192.168.1.22:5555and serial). These settings are transient and vary between developers, which can create unnecessary merge conflicts and commit noise.Additionally, this change is unrelated to the PR's stated objectives (Dashboard screen and Room database).
📝 Recommended approach
Add this file to
.gitignoreto prevent future commits:# IDE-specific deployment target selections .idea/deploymentTargetSelector.xmlThen remove it from version control:
git rm --cached .idea/deploymentTargetSelector.xmlmodules/lyrics-maker/src/main/res/layout/lyrics_container.xml (1)
111-139: Consider extracting hardcoded button dimensions.The three ImageButtons use hardcoded
60dpdimensions. While functional, consider extracting these todimens.xmlfor easier theming and consistency.♻️ Optional refactor
In
res/values/dimens.xml:<dimen name="control_button_size">60dp</dimen> <dimen name="control_button_margin">10dp</dimen>Then update the layout:
<ImageButton android:id="@+id/btn_previous" style="@style/Widget.AppCompat.Button.Borderless.Colored" - android:layout_width="60dp" - android:layout_height="60dp" - android:layout_margin="10dp" + android:layout_width="@dimen/control_button_size" + android:layout_height="@dimen/control_button_size" + android:layout_margin="@dimen/control_button_margin" android:background="@drawable/button_selector_circle" android:padding="16dp" android:src="@android:drawable/ic_media_previous" />Apply similar changes to
btn_play_pauseandbtn_next.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.idea/compiler.xml.idea/deploymentTargetSelector.xml.idea/misc.xmlmodules/app/src/main/java/com/tejpratapsingh/animator/app/MyApplication.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.ktmodules/lyrics-maker/src/main/res/drawable/button_selector_circle.xmlmodules/lyrics-maker/src/main/res/drawable/progress_bar_horizontal.xmlmodules/lyrics-maker/src/main/res/drawable/progress_bar_thumb_circle.xmlmodules/lyrics-maker/src/main/res/layout/lyrics_container.xml
✅ Files skipped from review due to trivial changes (4)
- modules/lyrics-maker/src/main/res/drawable/progress_bar_thumb_circle.xml
- modules/lyrics-maker/src/main/res/drawable/button_selector_circle.xml
- .idea/misc.xml
- modules/app/src/main/java/com/tejpratapsingh/animator/app/MyApplication.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build app
- GitHub Check: Build lyrics-maker
🔇 Additional comments (5)
.idea/compiler.xml (1)
4-4: Remove this comment—IDE configuration files in.idea/are not part of version control.The
.idea/compiler.xmlfile is IDE-local configuration (it's in.gitignore) and doesn't enforce project requirements. The actual Java compatibility is defined inbuild.gradlefiles usingsourceCompatibility/targetCompatibility(currently Java 8–11 across modules), not IDE settings.If you need to establish a consistent minimum Java version for the project, configure it in
gradle.propertiesorbuild.gradleinstead.Likely an incorrect or invalid review comment.
modules/lyrics-maker/src/main/res/drawable/progress_bar_horizontal.xml (1)
1-25: LGTM! Well-structured progress drawable.The drawable resource follows Android conventions and provides appropriate visual styling for the SeekBar.
modules/lyrics-maker/src/main/res/layout/lyrics_container.xml (3)
41-44: LGTM! Improved lyrics readability.The updated styling (cursive font, larger text sizes, and 2-line support for line1) enhances the visual presentation of lyrics.
Also applies to: 59-62
85-96: Confirm: Is hiding FakeAudioChartView intentional?The
FakeAudioChartViewis now hidden withandroid:visibility="gone". If this is a temporary change or the view is intended to be shown conditionally, please ensure the visibility logic is implemented in the Kotlin code.
70-83: Both drawable resources exist and are properly located in the module.
| private val tvLyricsLine1: TextView | ||
| private val tvLyricsLine2: TextView | ||
| private val progress: ProgressBar | ||
| private val progress: SeekBar |
There was a problem hiding this comment.
Handle SeekBar user interaction or disable it.
The SeekBar is interactive by default, allowing users to drag the thumb. However, no OnSeekBarChangeListener is attached to handle seeking behavior. This creates two potential issues:
- If seeking is intended: Add a listener to handle user interaction and update the video/animation position accordingly.
- If read-only display is intended: Disable user interaction to prevent confusion.
🔧 Proposed solutions
Solution 1: Disable interaction for read-only progress display
progress = view.findViewById(R.id.pb_progress)
+progress.isEnabled = false
fakeChartView = view.findViewById(R.id.fake_chart_view)Solution 2: Add listener for interactive seeking (if intended)
progress.progress = startFrame
progress.max = endFrame
+
+progress.setOnSeekBarChangeListener(object : SeekBar.OnSeekBarChangeListener {
+ override fun onProgressChanged(seekBar: SeekBar?, progress: Int, fromUser: Boolean) {
+ if (fromUser) {
+ // TODO: Implement seek logic - update video/animation to this frame
+ }
+ }
+ override fun onStartTrackingTouch(seekBar: SeekBar?) {}
+ override fun onStopTrackingTouch(seekBar: SeekBar?) {}
+})Also applies to: 60-60, 65-66, 127-127
🤖 Prompt for AI Agents
In
@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt
at line 46, The declared SeekBar field 'progress' (and other SeekBar instances
in this class) is interactive by default but has no listener or disablement;
decide whether it should be read-only or allow seeking: if read-only call
setEnabled(false) / setOnTouchListener { _, _ -> true } on 'progress' (and the
other SeekBars) to prevent user interaction, otherwise attach
setOnSeekBarChangeListener(...) to 'progress' and implement
onStartTrackingTouch/onStopTrackingTouch/onProgressChanged to update the
playback/animation position and sync UI state.
| <ImageButton | ||
| android:id="@+id/btn_previous" | ||
| style="@style/Widget.AppCompat.Button.Borderless.Colored" | ||
| android:layout_width="0dp" | ||
| android:layout_height="wrap_content" | ||
| android:layout_weight="1" | ||
| android:layout_width="60dp" | ||
| android:layout_height="60dp" | ||
| android:layout_margin="10dp" | ||
| android:background="@drawable/button_selector_circle" | ||
| android:padding="16dp" | ||
| android:src="@android:drawable/ic_media_previous" /> | ||
|
|
||
| <ImageButton | ||
| android:id="@+id/btn_play_pause" | ||
| style="@style/Widget.AppCompat.Button.Borderless.Colored" | ||
| android:layout_width="0dp" | ||
| android:layout_height="wrap_content" | ||
| android:layout_weight="1" | ||
| android:layout_width="60dp" | ||
| android:layout_height="60dp" | ||
| android:layout_margin="10dp" | ||
| android:background="@drawable/button_selector_circle" | ||
| android:padding="16dp" | ||
| android:src="@android:drawable/ic_media_pause" /> | ||
|
|
||
| <ImageButton | ||
| android:id="@+id/btn_next" | ||
| style="@style/Widget.AppCompat.Button.Borderless.Colored" | ||
| android:layout_width="0dp" | ||
| android:layout_height="wrap_content" | ||
| android:layout_weight="1" | ||
| android:layout_width="60dp" | ||
| android:layout_height="60dp" | ||
| android:layout_margin="10dp" | ||
| android:background="@drawable/button_selector_circle" | ||
| android:padding="16dp" | ||
| android:src="@android:drawable/ic_media_next" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Searching for all references to button IDs ==="
echo ""
echo "--- btn_previous references ---"
rg 'btn_previous' --type kotlin -B 2 -A 2
echo ""
echo "--- btn_play_pause references ---"
rg 'btn_play_pause' --type kotlin -B 2 -A 2
echo ""
echo "--- btn_next references ---"
rg 'btn_next' --type kotlin -B 2 -A 2
echo ""
echo "=== Checking for view binding patterns ==="
rg 'binding\.' --type kotlin | grep -E 'btn_(previous|play_pause|next)' || echo "No view binding found for these buttons"Repository: tejpratap46/AndroidVideoMotion
Length of output: 305
🏁 Script executed:
#!/bin/bash
echo "=== Searching for usage of lyrics_container.xml layout ==="
rg 'lyrics_container' --type kotlin -B 2 -A 2
echo ""
echo "=== Searching for layout references (all kotlin files) ==="
rg 'R\.layout\.lyrics' --type kotlin -B 2 -A 2
echo ""
echo "=== Checking for any Activity/Fragment using this layout ==="
rg 'setContentView|inflate.*lyrics' --type kotlin -B 2 -A 2
echo ""
echo "=== List all Kotlin files in the module ==="
find modules/lyrics-maker -name '*.kt' -type f 2>/dev/null | head -20Repository: tejpratap46/AndroidVideoMotion
Length of output: 6057
🏁 Script executed:
#!/bin/bash
echo "=== Full content of LyricsContainer.kt ==="
cat modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 4810
Implement button click handlers for media controls.
The three control buttons (btn_previous, btn_play_pause, btn_next) are defined in the layout but have no click handlers. They need to be wired with setOnClickListener() in LyricsContainer.kt or the Activity/Fragment that uses this view.
🤖 Prompt for AI Agents
In @modules/lyrics-maker/src/main/res/layout/lyrics_container.xml around lines
111 - 139, The layout defines ImageButtons btn_previous, btn_play_pause and
btn_next but they are not wired to actions; open LyricsContainer.kt (or the
hosting Activity/Fragment), find where the view is inflated/initialized and call
findViewById (or view binding) to get references to btn_previous, btn_play_pause
and btn_next and attach setOnClickListener handlers for each; inside those
listeners invoke the appropriate methods or callbacks (e.g., onPrevious(),
togglePlayPause()/onPlayPause(), onNext() or a provided MediaControlCallback) to
perform the media actions and update UI state (play/pause icon) as needed.
hemusimple
force-pushed
the
feat/add-dashboard
branch
from
ccf1a2d to
ea25fd9
Compare
There was a problem hiding this comment.
Actionable comments posted: 18
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/viewmodel/LyricsViewModel.kt (2)
80-90: Potential crash:first()called on potentially empty list.The getter accesses
field.first()without checking if the list is empty. SinceselectedLyricsis initialized asemptyList(), calling the getter before setting a value will throwNoSuchElementException.🐛 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 } }
71-78: Inconsistent usage ofprojectManagerinstance vsProjectManagersingleton.The constructor accepts
projectManageras a parameter (enabling testability), but lines 77, 93, and 99 reference theProjectManagersingleton directly instead of using the injectedprojectManagerinstance. This defeats the purpose of dependency injection and will cause issues in tests.♻️ Proposed fix
val lyrics: List<SyncedLyricFrame> get() = LrcHelper.getSyncedLyrics( lrcContent = selectedLyricResponse.getLyrics(), fps = MotionConfig.fps, ).also { - ProjectManager.songProject.totalLyrics = getSyncedLyricFrameStringList(it) + projectManager.songProject.totalLyrics = getSyncedLyricFrameStringList(it) } var selectedLyrics: List<SyncedLyricFrame> = emptyList() // ... getter ... set(value) { field = value - ProjectManager.songProject.apply { + projectManager.songProject.apply { selectedLyrics = field.joinToString(separator = ",") { - Log.d("ooo","formatted line ${it.line()}") it.line() } } - Log.d("ooo","finalSelected ${ProjectManager.songProject.selectedLyrics}") }Also applies to: 91-100
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt (1)
89-98: Potential crash:split(" - ")[1]may throwIndexOutOfBoundsException.If
songNamedoesn't contain " - " separator,split(" - ")[1]will crash. Also, the commented-out code should be removed.🐛 Proposed fix
} else { Log.i(TAG, "Fetching from musicbrainz") + val parts = songName.split(" - ", limit = 2) + if (parts.size >= 2) { AlbumArtFetcher .fetchAlbumArtUrl( - songName.split(" - ")[0], - songName.split(" - ")[1], + parts[0], + parts[1], )?.let { url -> Log.i(TAG, "cover art found: $url") ImageLoader.loadImage(context, url).into(this@apply) -// setImageBitmap(AlbumArtFetcher.fetchAlbumArtBitmap(url)) -// AlbumArtFetcher.close() } + } }
🤖 Fix all issues with AI agents
In @gradle/libs.versions.toml:
- Line 11: Update the KSP version entry in libs.versions.toml (the ksp = ...
entry) from 2.0.4 to 2.3.4 to pick up the latest stable KSP; also replace or
supplement the material_icons = "1.7.8" entry by migrating to Material Symbols
(introduce a material_symbols entry and update any dependencies and imports that
reference androidx.compose.material.icons to the corresponding Material Symbols
artifact and APIs) so the project uses the newer symbols library for future
compatibility.
In
@modules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/database/DatabaseProvider.kt:
- Around line 12-16: The Room database is built without a migration strategy in
DatabaseProvider when constructing ProjectDatabase (the instance created via
Room.databaseBuilder), causing upgrade crashes; update the builder chain to
either call .fallbackToDestructiveMigration() for dev/testing or supply
.addMigrations(...) with defined Migration objects for production (create
Migration instances targeting the appropriate version numbers and pass them to
addMigrations), ensuring the change is applied on the same builder that produces
the ProjectDatabase instance.
In
@modules/imageloader/src/main/java/com/tejpratapsingh/motion/imageloader/ImageLoader.kt:
- Around line 18-36: The coroutine in loadBitmap uses a CustomTarget but does
not override onLoadFailed, so failures never resume the continuation; update the
anonymous CustomTarget in loadBitmap to override onLoadFailed and call
cont.resumeWithException with an appropriate exception (or cont.cancel with a
Throwable) when Glide fails, keep onResourceReady calling cont.resume(resource)
and onLoadCleared cancelling if not completed, and ensure invokeOnCancellation
still clears the target via Glide.with(context).clear(target) so no leaks occur.
In @modules/lyrics-maker/src/main/AndroidManifest.xml:
- Around line 22-26: Remove the intent-filter that declares
android.intent.action.MAIN (and any DEFAULT/LAUNCHER category) from the
SearchActivity entry in the AndroidManifest; SearchActivity should only keep its
custom metadata-extractor intent-filter (if present) and be launched explicitly
via Intent(context, SearchActivity::class.java), while the MAIN/LAUNCHER
intent-filter must remain on DashboardActivity (the actual app entry point).
In
@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/AppNavHost.kt:
- Around line 62-71: The navigation argument parsing in the composable route
(the composable block using backStackEntry and getSyncedLyricFrameList) lacks
validation and can crash on malformed CSV; wrap the parsing in a try/catch,
validate itemsArg is non-null and non-empty, sanitize the split results (filter
blank entries, validate expected token format) before calling
getSyncedLyricFrameList, and on error log a warning and fall back to a safe
default (null or empty list). Remove or gate the debug Log.d calls and instead
log only on exceptions or invalid input. Consider replacing fragile CSV parsing
with a robust approach (URL-encoded JSON or NavType/Parcelable arguments) for
future-proofing and update callers accordingly.
- Line 43: The navigation call navController.navigate("${Screen.Lyrics.route}/")
produces "lyrics/" which does not match the declared route pattern
"${Screen.Lyrics.route}/{lyrics}" (lyrics/{lyrics}); fix by either (A) passing a
concrete lyrics argument when calling navController.navigate (e.g., build the
path using Screen.Lyrics.route and the actual lyrics string), (B) if you intend
to navigate without lyrics, make the lyrics parameter optional in the NavHost
route declaration (update the route pattern and add a navArgument/defaultValue
for the {lyrics} argument in the composable registration), or (C) explicitly
include an empty string as the path parameter so the navigated path matches the
placeholder (i.e., supply the parameter instead of just appending a trailing
slash).
In
@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/DashBoardScreen.kt:
- Around line 117-121: LaunchedEffect is currently keyed on project.url but
reads project.image and lacks error handling; change the key to project.image in
the LaunchedEffect(...) call and wrap the ImageLoader.loadBitmap(context, it)
call in a try-catch inside that effect (catch Exception), setting bitmap to null
or a fallback on failure and logging or reporting the exception; update
references to LaunchedEffect, project.image, ImageLoader.loadBitmap, bitmap and
context accordingly so image reloads when project.image changes and exceptions
don’t crash the composable.
In
@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetails.kt:
- Around line 118-154: LyricRow is incorrectly parsing "frame:text" and will
crash; replace the current extraction that uses substringAfter(":")[0/1] with
robust parsing: get the frame string via substringBefore(":"), convert it safely
to Int (e.g., toIntOrNull() with a fallback) and get the full lyric text via
substringAfter(":", "").trim(), handle empty or missing parts gracefully (use a
default frame 0 and default text ""), and update the creation of
SyncedLyricFrame(frame = <int>, text = <fullText>) accordingly so it no longer
reads single characters or throws on short strings.
- Around line 59-63: The image loading in the LaunchedEffect using
ImageLoader.loadBitmap(context, it) lacks error handling; wrap the call inside
LaunchedEffect(project.url) with a try/catch, set a new state flag (e.g.,
imageError) to true on failure and clear/set bitmap only on success, and
optionally log the exception; update the UI consumer of bitmap to check
imageError and show a placeholder or error icon when imageError is true instead
of silently showing nothing.
- Around line 50-63: ProjectDetails currently force-unwraps
lyricsViewModel.currentSelectedProject (using !!) which can crash; update
ProjectDetails to handle a null project by either accepting a non-null projectId
and loading the project inside the composable or by guarding the existing
access: replace val project = lyricsViewModel.currentSelectedProject!! with a
safe check (e.g., val project = lyricsViewModel.currentSelectedProject ?: run {
/* show error/empty state or return */ }) and ensure any usages
(LaunchedEffect(project.url), project.image, bitmap assignment) are inside the
null-safe branch so no property on a null object is accessed; alternatively use
requireNotNull with a clear message if you prefer failing fast.
In
@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt:
- Around line 91-100: Remove the debug Log.d calls in the setter (the anonymous
setter "set(value)" in LyricsViewModel) that use the "ooo" tag: delete the Log.d
inside the joinToString lambda and the final Log.d after setting
ProjectManager.songProject.selectedLyrics so the setter only assigns
ProjectManager.songProject.selectedLyrics = field.joinToString(...) without
logging sensitive lyrics content.
- Around line 113-121: The Log.d in loadDashboardData currently logs the result
of the ?.let chain (which is Unit) so savedData always shows "kotlin.Unit";
instead, capture the projects returned by projectManager.getAllProjects() into a
local val (e.g., val projects = projectManager.getAllProjects()) or move the
Log.d inside the let block after assigning _dashboardData.value = it, and log
that actual object (use Log.d("savedData", "$it") inside the let or
Log.d("savedData", "$projects") after the null check) so the real dashboard data
is logged rather than Unit.
In
@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/utils/ProjectUtils.kt:
- Around line 5-16: The getSyncedLyricFrameList function can crash on malformed
strings; change its implementation to validate and skip bad entries (use
mapNotNull instead of map), split each input with a single-limit split (so text
may contain colons), trim parts, convert the frame with toIntOrNull and skip if
null, and construct SyncedLyricFrame only for valid pairs; optionally log a
warning when skipping an entry to aid debugging.
In
@modules/metadata-extractor/src/main/java/com/tejpratapsingh/motion/metadataextractor/presentation/ShareReceiverActivity.kt:
- Around line 119-123: The loadImage method wraps a non-suspending Glide call in
lifecycleScope.launch unnecessarily; remove the coroutine and call
ImageLoader.loadImage(this@ShareReceiverActivity, url).into(binding.ivImage)
directly in ShareReceiverActivity.loadImage so threading is handled by Glide
itself and avoid the extra coroutine overhead from lifecycleScope.launch.
In
@modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/CurrentProject.kt:
- Around line 21-25: The toString() implementation in CurrentProject.toString()
contains a typo: it emits "instrument:$instrumental" instead of the correct key
"instrumental:$instrumental"; update the string concatenation in the toString()
method to use "instrumental:$instrumental" so the output key matches the field
name and other entries.
In
@modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/ProjectManager.kt:
- Around line 17-20: initDataBase currently assigns projectDatabase =
DatabaseProvider.getDatabase(context) and projectDao =
projectDatabase?.projectDao() but does not handle a null projectDatabase,
leading saveProject and getAllProjects to silently no-op; update initDataBase to
validate the result of DatabaseProvider.getDatabase(context) and if null either
throw an IllegalStateException (or return a boolean/Result) and log an explicit
error, or else set projectDao non-null by failing fast, so callers of
initDataBase know initialization failed and saveProject/getAllProjects won't
silently return null. Ensure you reference initDataBase,
DatabaseProvider.getDatabase, projectDatabase, projectDao, saveProject and
getAllProjects when applying the change.
- Around line 15-16: Replace the mutable shared var songProject with a
thread-safe StateFlow: create a private MutableStateFlow initialized with
CurrentProject() (e.g., _songProject = MutableStateFlow(CurrentProject())) and
expose a public read-only StateFlow (songProject: StateFlow<CurrentProject> =
_songProject.asStateFlow()); update all writers to use _songProject.value = ...
and readers to collect or read songProject.value. This change keeps
CurrentProject immutable at the public API, ensures visibility across threads,
and preserves existing callers by providing a .value accessor for synchronous
reads.
🧹 Nitpick comments (19)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SyncedLyricsSelector.kt (2)
53-53: Minor style: Remove space before colon.Kotlin convention is to avoid spaces before colons in parameter declarations.
✨ Suggested style fix
- lyricsList : List<SyncedLyricFrame>?, + lyricsList: List<SyncedLyricFrame>?,
62-66: Prefer idiomatic null-safe fallback with Elvis operator.The current if-else is correct but can be expressed more concisely using Kotlin's Elvis operator and
ifEmpty.♻️ More idiomatic approach
- val lyrics: List<SyncedLyricFrame> = if (lyricsList.isNullOrEmpty()){ - viewModel.lyrics - }else{ - lyricsList - } + val lyrics = lyricsList?.ifEmpty { null } ?: viewModel.lyricsmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/lrc/SyncedLyricFrame.kt (1)
12-14: Prefer Kotlin string templates over String.format.For simple formatting operations, Kotlin string templates are more idiomatic and readable than Java's
String.format.♻️ More idiomatic Kotlin approach
) : Parcelable{ - fun line() = String.format("%d:%s",frame,text) + fun line() = "$frame:$text" }modules/imageloader/src/test/java/com/tejpratapsingh/motion/imageloader/ExampleUnitTest.kt (1)
1-16: Consider replacing the placeholder test with a minimalImageLoaderbehavior test (or drop it).The
2 + 2test is consistent with other modules but doesn’t validate this module’s value; swapping it for a small unit test around your API (even a pure JVM test for parameter validation) would be more useful.modules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/dao/ProjectDao.kt (2)
15-16: Consider using Flow for reactive updates and better memory efficiency.Returning
List<ProjectEntity>loads all projects into memory at once, which could cause performance issues as the dataset grows. Room best practices recommend usingFlow<List<ProjectEntity>>for reactive queries that automatically update observers when data changes.♻️ Suggested refactor to use Flow
- @Query("SELECT * FROM projects") - suspend fun getAll(): List<ProjectEntity> + @Query("SELECT * FROM projects") + fun getAll(): Flow<List<ProjectEntity>>Note: Remove
suspendwhen usingFlowas the Flow itself handles async operations.
10-17: Consider adding query operations for individual projects.The DAO currently only supports bulk operations. For dashboard features like viewing or editing a specific project, you may need query operations such as
getById(id: Long)or delete operations.Example operations that might be useful
@Query("SELECT * FROM projects WHERE id = :projectId") suspend fun getById(projectId: Long): ProjectEntity? @Delete suspend fun delete(projectEntity: ProjectEntity) @Query("DELETE FROM projects WHERE id = :projectId") suspend fun deleteById(projectId: Long)modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/mapper/Mapper.kt (1)
6-42: Consider using data class copy or a mapping library for maintainability.The manual field-by-field mapping is verbose with 17 fields. For better maintainability as the schema evolves, consider:
- Using Kotlin data class
.copy()if both types are data classes- Using a mapping library like MapStruct or ModelMapper for automatic field mapping
This would reduce boilerplate and make future field additions less error-prone.
modules/imageloader/src/main/java/com/tejpratapsingh/motion/imageloader/ImageLoader.kt (1)
13-37: Consider the naming to avoid confusion with the existing ImageLoader interface.There's an existing
ImageLoaderinterface inmodules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/domain/ImageLoader.kt. While they're in different packages and won't conflict technically, having two types with the same name could cause confusion for developers. Consider renaming this object to something more specific likeGlideImageLoaderor coordinate with the sdui module's ImageLoader interface.modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/ProjectManager.kt (2)
21-24: Consider production logging practices.Line 22 uses
Log.dfor saving data. If this log contains sensitive user data or is too verbose for production, consider using a more appropriate log level or removing it for release builds.
25-27: Consider returning an empty list instead of null.The nullable return type
List<CurrentProject>?requires callers to handle null cases. If the intent is to return an empty list when there are no projects, consider returningemptyList()instead of null for a cleaner API:suspend fun getAllProjects(): List<CurrentProject> = projectDao?.getAll()?.map { it.toCurrentProject() } ?: emptyList()This eliminates null checks at call sites unless you specifically want to distinguish "DAO not initialized" from "no projects".
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/DashBoardScreen.kt (1)
115-143: Consider memory management for large project lists.Each
ProjectCardloads and holds a fullBitmapin memory. For lists with many projects, this could cause memory pressure. Consider these optimizations:
- Use Glide's built-in
AsyncImageor similar composables that handle memory caching automatically.- Limit bitmap size using Glide's
.override(width, height)to match the card's display size.- Rely on Glide's memory cache instead of storing bitmaps in composable state.
Example with Glide's built-in memory management:
// Use Glide RequestBuilder directly in AsyncImage or similar // This avoids holding bitmaps in composable stateNote: Compose will dispose off-screen cards, which helps, but Glide's built-in caching is more efficient.
modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/CurrentProject.kt (1)
3-20: Consider using data class with immutable fields.This class serves as a domain model and would benefit from:
- Using
data classfor automaticequals(),hashCode(), andcopy()implementations- Using
valinstead ofvarfor immutability and thread safety- Consider which fields should be non-nullable with sensible defaults
Immutable domain models are easier to reason about and safer in concurrent contexts.
♻️ Refactor to data class with immutable fields
-class CurrentProject( - var title: String? = null, - var description: String? = null, - var image: String? = null, - var siteName: String? = null, - var twitterCard: String? = null, - var url: String? = null, - var trackName: String? = null, - var artistName: String? = null, - var albumName: String? = null, - var duration: Float? = null, - var instrumental: Boolean? = null, - var plainLyrics: String? = null, - var syncedLyrics: String? = null, - var savedFilePath: String? = null, - var totalLyrics: String? = null, - var selectedLyrics: String? = null, +data class CurrentProject( + val title: String? = null, + val description: String? = null, + val image: String? = null, + val siteName: String? = null, + val twitterCard: String? = null, + val url: String? = null, + val trackName: String? = null, + val artistName: String? = null, + val albumName: String? = null, + val duration: Float? = null, + val instrumental: Boolean? = null, + val plainLyrics: String? = null, + val syncedLyrics: String? = null, + val savedFilePath: String? = null, + val totalLyrics: String? = null, + val selectedLyrics: String? = null, ) { - override fun toString(): String { - return "title:$title, description:$description, image:$image, siteName:$siteName, twitterCard:$twitterCard, url:$url, trackName:$trackName," + - "artistName:$artistName, albumName:$albumName, duration:$duration, instrument:$instrumental, plainLyrics:$plainLyrics, syncedLyrics:$syncedLyrics," + - "savedFilePath:$savedFilePath, totalLyrics:$totalLyrics, selectedLyrics:$selectedLyrics" - } }Note: If you switch to
data class, you can remove the customtoString()as it will be generated automatically.modules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/entity/ProjectEntity.kt (2)
6-26: Consider adding database indexes for frequently queried fields.If you plan to query projects by fields like
url,trackName,savedFilePath, orartistName, add indexes to improve query performance. For example:@Entity( tableName = "projects", indices = [ Index(value = ["url"], unique = true), Index(value = ["savedFilePath"]), Index(value = ["trackName", "artistName"]) ] )Additionally, consider whether some fields should be non-nullable with default values (e.g.,
instrumentalcould default tofalse).
26-26: Add trailing newline at end of file.The file is missing a trailing newline, which is a common convention in many coding style guides and helps with version control diffs.
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt (3)
31-48: Consider removing or reducing debug logging for production.The
Log.dstatements on lines 34 and 44 appear to be debug artifacts. Consider removing them or wrapping in aBuildConfig.DEBUGcheck to avoid logging potentially sensitive data (like song titles, URLs) in production builds.♻️ Suggested improvement
init { viewModelScope.launch { socialMeta.collect { socialMeta -> - Log.d("receive socialMeta", "$socialMeta") socialMeta?.let { with(projectManager.songProject) { title = it.title description = it.description image = it.image siteName = it.siteName twitterCard = it.twitterCard url = it.url } - Log.d("receive songProject", "${projectManager.songProject}") } } } }
104-112: Consider adding error handling for network failures.The
fetchLyrics()function doesn't handle exceptions fromclient.searchLyrics(). A network failure will crash and leaveisLoadingstuck attrue.♻️ Proposed improvement
suspend fun fetchLyrics() { isLoading.value = true - val results = - client.searchLyrics(SearchQuery(query.value)).filter { - it.syncedLyrics != null - } - _lyricsList.emit(results) - isLoading.value = false + try { + val results = + client.searchLyrics(SearchQuery(query.value)).filter { + it.syncedLyrics != null + } + _lyricsList.emit(results) + } catch (e: Exception) { + Log.e("LyricsViewModel", "Failed to fetch lyrics", e) + _lyricsList.emit(emptyList()) + } finally { + isLoading.value = false + } }
102-102: Minor formatting: missing space before=.-var currentSelectedProject: CurrentProject?=null +var currentSelectedProject: CurrentProject? = nullmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt (2)
32-33: Unused imports:runBlockingandsuspendCoroutine.These imports appear to be leftovers from the refactoring and are no longer used in the file. Consider removing them.
♻️ Proposed fix
import kotlinx.coroutines.MainScope import kotlinx.coroutines.cancel import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.launch -import kotlinx.coroutines.runBlocking -import kotlin.coroutines.suspendCoroutine
85-85: Minor: inconsistent spacing after commas.-ImageLoader.loadImage(context,image).into(this@apply) +ImageLoader.loadImage(context, image).into(this@apply)-ImageLoader.loadImage(context,url).into(this@apply) +ImageLoader.loadImage(context, url).into(this@apply)Also applies to: 95-95
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
gradle/libs.versions.tomlmodules/app/build.gradlemodules/datastore/.gitignoremodules/datastore/build.gradlemodules/datastore/consumer-rules.promodules/datastore/proguard-rules.promodules/datastore/src/main/AndroidManifest.xmlmodules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/dao/ProjectDao.ktmodules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/database/DatabaseProvider.ktmodules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/database/ProjectDatabase.ktmodules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/entity/ProjectEntity.ktmodules/datastore/src/test/java/com/tejpratapsingh/motion/datastore/ExampleUnitTest.ktmodules/imageloader/build.gradlemodules/imageloader/consumer-rules.promodules/imageloader/proguard-rules.promodules/imageloader/src/main/AndroidManifest.xmlmodules/imageloader/src/main/java/com/tejpratapsingh/motion/imageloader/ImageLoader.ktmodules/imageloader/src/test/java/com/tejpratapsingh/motion/imageloader/ExampleUnitTest.ktmodules/lyrics-maker/build.gradlemodules/lyrics-maker/src/main/AndroidManifest.xmlmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/LyricMaker.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/lrc/LrcHelper.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/lrc/SyncedLyricFrame.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/DashboardActivity.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/AppNavHost.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/DashBoardScreen.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetails.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SyncedLyricsSelector.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/utils/ProjectUtils.ktmodules/metadata-extractor/build.gradlemodules/metadata-extractor/src/main/java/com/tejpratapsingh/motion/metadataextractor/presentation/ShareReceiverActivity.ktmodules/ongoing/.gitignoremodules/ongoing/build.gradlemodules/ongoing/consumer-rules.promodules/ongoing/proguard-rules.promodules/ongoing/src/main/AndroidManifest.xmlmodules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/CurrentProject.ktmodules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/ProjectManager.ktmodules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/mapper/Mapper.ktmodules/ongoing/src/test/java/com/tejpratapsingh/motion/ongoing/ExampleUnitTest.ktsettings.gradle
🚧 Files skipped from review as they are similar to previous changes (14)
- modules/ongoing/proguard-rules.pro
- modules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/database/ProjectDatabase.kt
- modules/ongoing/.gitignore
- settings.gradle
- modules/datastore/proguard-rules.pro
- modules/datastore/build.gradle
- modules/ongoing/src/test/java/com/tejpratapsingh/motion/ongoing/ExampleUnitTest.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/lrc/LrcHelper.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/DashboardActivity.kt
- modules/datastore/.gitignore
- modules/datastore/src/test/java/com/tejpratapsingh/motion/datastore/ExampleUnitTest.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt
- modules/datastore/src/main/AndroidManifest.xml
- modules/ongoing/src/main/AndroidManifest.xml
🧰 Additional context used
🧬 Code graph analysis (9)
modules/metadata-extractor/src/main/java/com/tejpratapsingh/motion/metadataextractor/presentation/ShareReceiverActivity.kt (2)
modules/metadata-extractor/src/main/java/com/tejpratapsingh/motion/metadataextractor/presentation/MetaDataViewModel.kt (1)
downloadImage(30-30)modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/domain/ImageLoader.kt (2)
load(5-10)load(6-9)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/lrc/SyncedLyricFrame.kt (2)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/lrc/LrcLine.kt (1)
time(3-6)modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsTextView.kt (2)
forFrame(25-32)context(10-33)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/LyricMaker.kt (1)
modules/app/src/main/java/com/tejpratapsingh/animator/app/MyApplication.kt (1)
onCreate(7-15)
modules/imageloader/src/main/java/com/tejpratapsingh/motion/imageloader/ImageLoader.kt (1)
modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/domain/ImageLoader.kt (2)
load(5-10)load(6-9)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/DashBoardScreen.kt (1)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SearchLyricsCompose.kt (2)
SearchScreen(37-192){}(41-41)
modules/imageloader/src/test/java/com/tejpratapsingh/motion/imageloader/ExampleUnitTest.kt (10)
modules/motionlib/src/test/java/com/tejpratapsingh/motionlib/ExampleUnitTest.kt (1)
addition_isCorrect(11-16)modules/metadata-extractor/src/test/java/com/tejpratapsingh/motion/metadataextractor/ExampleUnitTest.kt (1)
addition_isCorrect(11-16)modules/jcodec-motion-ext/src/test/java/com/tejpratapsingh/motionlib/jcodec/ExampleUnitTest.kt (1)
addition_isCorrect(11-16)modules/core/src/test/java/com/tejpratapsingh/motionlib/core/ExampleUnitTest.kt (1)
addition_isCorrect(11-16)modules/pytorch-motion-ext/src/test/java/com/tejpratapsingh/motionlib/pytorch/ExampleUnitTest.kt (1)
addition_isCorrect(11-16)modules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/ExampleUnitTest.kt (1)
addition_isCorrect(11-16)modules/3d-filament-renderer/src/test/java/com/tejpratapsingh/motionlib/filamentrenderer/ExampleUnitTest.kt (1)
addition_isCorrect(11-16)modules/tensorflow-motion-ext/src/test/java/com/tejpratapsingh/motionlib/tensorflow/ExampleUnitTest.kt (1)
addition_isCorrect(11-16)modules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/ExampleUnitTest.kt (1)
addition_isCorrect(11-16)modules/ffmpeg-motion-ext/src/test/java/com/tejpratapsingh/motionlib/ffmpeg/ExampleUnitTest.kt (1)
addition_isCorrect(11-16)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt (2)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/client/AlbumArtFetcher.kt (1)
client(18-85)modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/domain/ImageLoader.kt (1)
load(5-10)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.kt (1)
modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/ProjectManager.kt (1)
saveProject(21-24)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/LyricsViewModel.kt (1)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/utils/ProjectUtils.kt (1)
getSyncedLyricFrameStringList(18-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build app
- GitHub Check: Build lyrics-maker
🔇 Additional comments (21)
modules/imageloader/src/main/AndroidManifest.xml (1)
1-2: LGTM! Standard library module manifest.This placeholder manifest is appropriate for an Android library module that doesn't require special manifest declarations.
modules/imageloader/proguard-rules.pro (1)
1-21: If this module is published/consumed, ensure consumer ProGuard rules are wired (or remove the unused template).Right now it’s only comments; that’s fine, but make sure
modules/imageloader/build.gradlereferences it viaconsumerProguardFilesif the intent is to protect consumers when minified.modules/metadata-extractor/build.gradle (1)
48-56: Dependency direction is correct; no issues found.No circular dependencies detected (metadata-extractor → imageloader does not create a cycle). Metadata-extractor does not expose imageloader types in its public API, confirming that
implementationis the appropriate scope rather thanapi.modules/app/build.gradle (1)
43-48: Removelibs.material.iconsfrom modules/app—this is a Compose-only dependency not used here.This module has no Compose dependencies and contains no icon usage. The dependency appears to be inadvertently copied from
modules:lyrics-maker(which does use Compose and icons). Keeping it adds unnecessary bloat to the app module.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/LyricMaker.kt (1)
6-10: No manifest merge conflict exists; both are independent application modules.
modules/app(applicationId:com.tejpratapsingh.animator) andmodules/lyrics-maker(applicationId:com.tejpratapsingh.lyricsmaker) are separate applications with different package names. The app module does not depend on lyrics-maker, so no manifest merge will occur. Additionally,ProjectManager.initDataBase()is already idempotent—DatabaseProvider.getDatabase()uses a proper singleton pattern with@Volatileand synchronized locking, ensuring the database is initialized only once even if called multiple times.modules/lyrics-maker/build.gradle (1)
60-69: LGTM! Dependencies correctly added.The new module dependencies and material icons library are properly scoped as
implementation, supporting the dashboard and image loading features introduced in this PR.modules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/database/DatabaseProvider.kt (1)
10-19: LGTM! Thread-safe singleton pattern correctly implemented.The double-checked locking with
@Volatileandsynchronizedproperly ensures thread-safe lazy initialization of the database instance.modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/mapper/Mapper.kt (1)
6-23: No changes needed.The mapper correctly handles Room's auto-generated ID. Since
CurrentProjecthas no ID field, the mapper intentionally omits it, allowing Room to auto-generate a new ID when inserting (withautoGenerate = true). This is the correct pattern for creating new entities.Likely an incorrect or invalid review comment.
modules/imageloader/build.gradle (1)
46-46: Theapiscope is necessary—do not change toimplementation.The
ImageLoader.loadImage()method returns a GlideRequestBuilderobject directly, and consumers chain Glide methods on it (e.g.,.into(target)). Changing toimplementationscope would break compilation for modules likelyrics-makerandmetadata-extractor, which depend on Glide types to use the return value.If encapsulation is desired, refactor
loadImage()to accept a callback/target parameter instead of returning theRequestBuilder. The current API design requires exposing Glide, makingapithe correct scope.Likely an incorrect or invalid review comment.
modules/lyrics-maker/src/main/AndroidManifest.xml (2)
10-10: LGTM: Custom Application class declaration.The custom Application class is properly declared and aligns with the initialization logic in LyricMaker.kt.
34-43: LGTM: DashboardActivity launcher configuration.The new DashboardActivity is properly configured as the app's launcher entry point with the correct intent-filter.
modules/imageloader/src/main/java/com/tejpratapsingh/motion/imageloader/ImageLoader.kt (1)
15-16: LGTM: Simple Glide wrapper.The loadImage function provides a clean wrapper around Glide's image loading API.
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/DashBoardScreen.kt (1)
52-107: LGTM: Dashboard screen implementation.The DashBoardScreen composable is well-structured with proper state management, one-time data loading, and clean navigation logic. The FAB with dropdown menu provides a good UX for creating new projects.
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/utils/ProjectUtils.kt (1)
18-19: No changes needed. Theline()method exists and correctly returns the expected "frame:text" format (e.g., "0:Hello"). The comma delimiter is appropriate since commas separate frame entries while colons separate frame numbers from text within each entry—text containing commas will parse correctly.modules/ongoing/build.gradle (1)
1-47: LGTM! Build configuration is clean and follows best practices.The Gradle configuration properly sets up the Android library module with:
- Appropriate SDK versions (compileSdk 36, minSdk 25)
- Java 11 compatibility
- Maven publishing for module distribution
- Clean dependency on the datastore module
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/AppNavHost.kt (2)
24-25: LGTM! Clean addition of new navigation destinations.The new
DashboardandProjectDetailsscreen objects follow the existing navigation pattern correctly.
48-60: LGTM! New routes are properly configured.The Dashboard and ProjectDetails composable destinations are correctly wired with appropriate parameters passed to each screen.
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetails.kt (1)
112-114: CSV format for lyrics is fragile and can break if lyric text contains commas.Line 112 uses comma-splitting to parse
selectedLyrics. If any lyric contains a comma, the parsing will fail. The review also flags potential format inconsistency withAppNavHost.kt.Consider using a more robust serialization format like JSON array, or define a delimiter that won't appear in lyric text.
gradle/libs.versions.toml (2)
86-90: LGTM! Room library declarations are well-organized.The Room dependencies are correctly declared with consistent formatting and proper version references. Including
room-testingis good practice for unit testing Room components.
100-100: LGTM!The KSP plugin declaration is correctly formatted and essential for Room annotation processing.
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt (1)
140-143: LGTM! Proper lifecycle cleanup.Good implementation of
onDetachedFromWindow()to cancel the coroutine scope, preventing memory leaks and cancelled work from continuing after the view is detached.
| ktx_version = "1.17.0" | ||
| appcompat_version = "1.7.1" | ||
| material_version = "1.13.0" | ||
| material_icons = "1.7.8" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
latest stable androidx.room version 2025 2026
💡 Result:
The latest stable AndroidX Room release is 2.8.4 (released November 19, 2025). [1][2]
Sources:
[1] Android Developers — Room release notes.
[2] Maven Repository — androidx.room:room-runtime (latest).
🌐 Web query:
latest stable KSP Kotlin Symbol Processing version 2025 2026
💡 Result:
The latest stable KSP release is 2.3.4 (released Dec 16, 2025). [1][2]
Sources:
- google/ksp releases (GitHub) — 2.3.4. [1]
- Kotlin KSP documentation (Kotlinlang) — KSP2 notes. [2]
🌐 Web query:
latest stable Glide image loading library version 2025 2026
💡 Result:
Latest stable Glide (bumptech/glide) release is v5.0.5 (released Sep 13, 2025). [1][2]
Sources:
- bumptech/glide Releases (v5.0.5). [1]
- Glide documentation / download page. [2]
🌐 Web query:
latest stable androidx material icons version 2025 2026
💡 Result:
Latest stable androidx Material Icons: 1.7.8 (released Feb 12, 2025). [1][2]
Note: the androidx.compose.material.icons library is no longer recommended (Material Symbols preferred). [1]
Sources:
- AndroidX Compose Material release notes (library deprecation). [1]
- Maven / artifact listing for material-icons-extended / material-icons-core (1.7.8). [2]
Update KSP to 2.3.4 and consider Material Symbols instead of Material Icons.
Room 2.8.4, Glide 5.0.5, and Material Icons 1.7.8 are latest stable. However, KSP 2.0.4 is outdated; the latest stable is 2.3.4 (released Dec 16, 2025). Update the KSP version accordingly.
Additionally, androidx.compose.material.icons (Material Icons) is deprecated in favor of Material Symbols. If possible, migrate to Material Symbols for future compatibility.
🤖 Prompt for AI Agents
In @gradle/libs.versions.toml at line 11, Update the KSP version entry in
libs.versions.toml (the ksp = ... entry) from 2.0.4 to 2.3.4 to pick up the
latest stable KSP; also replace or supplement the material_icons = "1.7.8" entry
by migrating to Material Symbols (introduce a material_symbols entry and update
any dependencies and imports that reference androidx.compose.material.icons to
the corresponding Material Symbols artifact and APIs) so the project uses the
newer symbols library for future compatibility.
| val instance = Room.databaseBuilder( | ||
| context.applicationContext, | ||
| ProjectDatabase::class.java, | ||
| "LyricMaker" | ||
| ).build() |
There was a problem hiding this comment.
Add database migration strategy to prevent crashes on schema changes.
The Room database is built without a migration strategy. If the schema changes in future releases, the app will crash when users upgrade. You must either:
- Add
.fallbackToDestructiveMigration()for development/testing (destroys data on schema changes) - Implement proper migrations with
.addMigrations()for production (preserves user data)
🔧 Proposed fix options
Option 1: For development (destroys data on schema changes):
val instance = Room.databaseBuilder(
context.applicationContext,
ProjectDatabase::class.java,
"LyricMaker"
-)
+)
+.fallbackToDestructiveMigration()
.build()Option 2: For production (preserves data with migrations):
val instance = Room.databaseBuilder(
context.applicationContext,
ProjectDatabase::class.java,
"LyricMaker"
-)
+)
+.addMigrations(MIGRATION_1_2, MIGRATION_2_3, /* etc */)
.build()You'll need to define migration objects separately. See Room Migration documentation.
📝 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.
| val instance = Room.databaseBuilder( | |
| context.applicationContext, | |
| ProjectDatabase::class.java, | |
| "LyricMaker" | |
| ).build() | |
| val instance = Room.databaseBuilder( | |
| context.applicationContext, | |
| ProjectDatabase::class.java, | |
| "LyricMaker" | |
| ) | |
| .fallbackToDestructiveMigration() | |
| .build() |
🤖 Prompt for AI Agents
In
@modules/datastore/src/main/java/com/tejpratapsingh/motion/datastore/data/database/DatabaseProvider.kt
around lines 12 - 16, The Room database is built without a migration strategy in
DatabaseProvider when constructing ProjectDatabase (the instance created via
Room.databaseBuilder), causing upgrade crashes; update the builder chain to
either call .fallbackToDestructiveMigration() for dev/testing or supply
.addMigrations(...) with defined Migration objects for production (create
Migration instances targeting the appropriate version numbers and pass them to
addMigrations), ensuring the change is applied on the same builder that produces
the ProjectDatabase instance.
| suspend fun loadBitmap(context: Context, url: String): Bitmap = suspendCancellableCoroutine { cont -> | ||
| val target = object : CustomTarget<Bitmap>() { | ||
| override fun onResourceReady(resource: Bitmap, transition: Transition<in Bitmap>?) { | ||
| cont.resume(resource) | ||
| } | ||
| override fun onLoadCleared(placeholder: Drawable?) { | ||
| if (!cont.isCompleted) cont.cancel() | ||
| } | ||
| } | ||
|
|
||
| Glide.with(context) | ||
| .asBitmap() | ||
| .load(url) | ||
| .into(target) | ||
|
|
||
| cont.invokeOnCancellation { | ||
| Glide.with(context).clear(target) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for failed image loads.
The loadBitmap function lacks error handling. If Glide fails to load the image (network error, 404, invalid URL, etc.), the CustomTarget does not override onLoadFailed, so the coroutine continuation is never resumed, causing the caller to hang indefinitely.
🐛 Proposed fix to handle load failures
suspend fun loadBitmap(context: Context, url: String): Bitmap = suspendCancellableCoroutine { cont ->
val target = object : CustomTarget<Bitmap>() {
override fun onResourceReady(resource: Bitmap, transition: Transition<in Bitmap>?) {
cont.resume(resource)
}
override fun onLoadCleared(placeholder: Drawable?) {
if (!cont.isCompleted) cont.cancel()
}
+ override fun onLoadFailed(errorDrawable: Drawable?) {
+ cont.cancel(Exception("Failed to load image from URL: $url"))
+ }
}
Glide.with(context)
.asBitmap()
.load(url)
.into(target)
cont.invokeOnCancellation {
Glide.with(context).clear(target)
}
}📝 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.
| suspend fun loadBitmap(context: Context, url: String): Bitmap = suspendCancellableCoroutine { cont -> | |
| val target = object : CustomTarget<Bitmap>() { | |
| override fun onResourceReady(resource: Bitmap, transition: Transition<in Bitmap>?) { | |
| cont.resume(resource) | |
| } | |
| override fun onLoadCleared(placeholder: Drawable?) { | |
| if (!cont.isCompleted) cont.cancel() | |
| } | |
| } | |
| Glide.with(context) | |
| .asBitmap() | |
| .load(url) | |
| .into(target) | |
| cont.invokeOnCancellation { | |
| Glide.with(context).clear(target) | |
| } | |
| } | |
| suspend fun loadBitmap(context: Context, url: String): Bitmap = suspendCancellableCoroutine { cont -> | |
| val target = object : CustomTarget<Bitmap>() { | |
| override fun onResourceReady(resource: Bitmap, transition: Transition<in Bitmap>?) { | |
| cont.resume(resource) | |
| } | |
| override fun onLoadCleared(placeholder: Drawable?) { | |
| if (!cont.isCompleted) cont.cancel() | |
| } | |
| override fun onLoadFailed(errorDrawable: Drawable?) { | |
| cont.resumeWithException(Exception("Failed to load image from URL: $url")) | |
| } | |
| } | |
| Glide.with(context) | |
| .asBitmap() | |
| .load(url) | |
| .into(target) | |
| cont.invokeOnCancellation { | |
| Glide.with(context).clear(target) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
@modules/imageloader/src/main/java/com/tejpratapsingh/motion/imageloader/ImageLoader.kt
around lines 18 - 36, The coroutine in loadBitmap uses a CustomTarget but does
not override onLoadFailed, so failures never resume the continuation; update the
anonymous CustomTarget in loadBitmap to override onLoadFailed and call
cont.resumeWithException with an appropriate exception (or cont.cancel with a
Throwable) when Glide fails, keep onResourceReady calling cont.resume(resource)
and onLoadCleared cancelling if not completed, and ensure invokeOnCancellation
still clears the target via Glide.with(context).clear(target) so no leaks occur.
| <intent-filter> | ||
| <action android:name="android.intent.action.MAIN" /> | ||
|
|
||
| <category android:name="android.intent.category.LAUNCHER" /> | ||
| <category android:name="android.intent.category.DEFAULT" /> | ||
| </intent-filter> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how SearchActivity is launched and referenced in the codebase
echo "=== Searching for SearchActivity references in Kotlin files ==="
rg -n "SearchActivity" --type=kotlin -A 2 -B 2
echo ""
echo "=== Searching for SearchActivity in Java files ==="
rg -n "SearchActivity" --type=java -A 2 -B 2
echo ""
echo "=== Checking the complete AndroidManifest.xml for SearchActivity ==="
fd "AndroidManifest.xml" -x cat {}Repository: tejpratap46/AndroidVideoMotion
Length of output: 10598
Remove the MAIN action from SearchActivity's intent-filter.
SearchActivity is launched explicitly via Intent(context, SearchActivity::class.java) from DashBoardScreen and does not require the MAIN action. The MAIN action should only be on DashboardActivity (which is the actual app entry point with LAUNCHER category). Remove the intent-filter with the MAIN action and keep only the custom action intent-filter for the metadata extractor.
🤖 Prompt for AI Agents
In @modules/lyrics-maker/src/main/AndroidManifest.xml around lines 22 - 26,
Remove the intent-filter that declares android.intent.action.MAIN (and any
DEFAULT/LAUNCHER category) from the SearchActivity entry in the AndroidManifest;
SearchActivity should only keep its custom metadata-extractor intent-filter (if
present) and be launched explicitly via Intent(context,
SearchActivity::class.java), while the MAIN/LAUNCHER intent-filter must remain
on DashboardActivity (the actual app entry point).
| onLyricsSelected = { | ||
| viewModel.selectedLyricResponse = it | ||
| navController.navigate(Screen.Lyrics.route) | ||
| navController.navigate("${Screen.Lyrics.route}/") |
There was a problem hiding this comment.
Navigation route mismatch: trailing slash won't match route pattern.
Line 43 navigates to "${Screen.Lyrics.route}/" which becomes "lyrics/", but the route pattern at line 62 is "${Screen.Lyrics.route}/{lyrics}" ("lyrics/{lyrics}"). This navigation won't match the defined route.
Consider either:
- Passing an empty string explicitly:
navController.navigate("${Screen.Lyrics.route}/") - Making the lyrics parameter optional in the route definition
- Passing the actual lyrics as an argument
🔧 Possible fix
If you want to navigate without lyrics, update the route pattern:
- composable(route = "${Screen.Lyrics.route}/{lyrics}") { backStackEntry ->
+ composable(route = "${Screen.Lyrics.route}/{lyrics}",
+ arguments = listOf(navArgument("lyrics") {
+ type = NavType.StringType
+ defaultValue = ""
+ })
+ ) { backStackEntry ->Or pass actual lyrics data:
onLyricsSelected = {
viewModel.selectedLyricResponse = it
- navController.navigate("${Screen.Lyrics.route}/")
+ // Serialize and pass lyrics data
+ val lyricsArg = viewModel.getSyncedLyrics().joinToString(",") {
+ "${it.frame}:${it.text}"
+ }
+ navController.navigate("${Screen.Lyrics.route}/$lyricsArg")
},🤖 Prompt for AI Agents
In
@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/AppNavHost.kt
at line 43, The navigation call
navController.navigate("${Screen.Lyrics.route}/") produces "lyrics/" which does
not match the declared route pattern "${Screen.Lyrics.route}/{lyrics}"
(lyrics/{lyrics}); fix by either (A) passing a concrete lyrics argument when
calling navController.navigate (e.g., build the path using Screen.Lyrics.route
and the actual lyrics string), (B) if you intend to navigate without lyrics,
make the lyrics parameter optional in the NavHost route declaration (update the
route pattern and add a navArgument/defaultValue for the {lyrics} argument in
the composable registration), or (C) explicitly include an empty string as the
path parameter so the navigated path matches the placeholder (i.e., supply the
parameter instead of just appending a trailing slash).
| fun getSyncedLyricFrameList(data: List<String>) = | ||
| if (data.isEmpty()){ | ||
| emptyList() | ||
| }else{ | ||
| data.map { | ||
| val (frame, text) = it.split(":") | ||
| SyncedLyricFrame( | ||
| frame = frame.toInt(), | ||
| text = text | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling and input validation.
The getSyncedLyricFrameList function has several error-prone operations without validation:
- Line 10:
split(":")followed by destructuring(frame, text)will throw if the string doesn't contain exactly one colon. - Line 12:
frame.toInt()will throwNumberFormatExceptionif the frame portion is not a valid integer. - Line 10: If text contains colons (e.g., "100:Hello:World"), the split will produce more than 2 elements and destructuring will fail.
These unhandled exceptions can crash the app when processing malformed data.
🐛 Proposed fix with error handling
fun getSyncedLyricFrameList(data: List<String>) =
if (data.isEmpty()){
emptyList()
}else{
- data.map {
- val (frame, text) = it.split(":")
+ data.mapNotNull {
+ val parts = it.split(":", limit = 2)
+ if (parts.size != 2) return@mapNotNull null
+ val frameNum = parts[0].toIntOrNull() ?: return@mapNotNull null
SyncedLyricFrame(
- frame = frame.toInt(),
- text = text
+ frame = frameNum,
+ text = parts[1]
)
}
}Note: Changed to mapNotNull to filter out invalid entries rather than crashing. Consider logging warnings for skipped entries.
🤖 Prompt for AI Agents
In
@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/utils/ProjectUtils.kt
around lines 5 - 16, The getSyncedLyricFrameList function can crash on malformed
strings; change its implementation to validate and skip bad entries (use
mapNotNull instead of map), split each input with a single-limit split (so text
may contain colons), trim parts, convert the frame with toIntOrNull and skip if
null, and construct SyncedLyricFrame only for valid pairs; optionally log a
warning when skipping an entry to aid debugging.
| private fun loadImage(url: String) { | ||
| lifecycleScope.launch { | ||
| val image = metadataViewModel.downloadImage(url) | ||
| binding.ivImage.setImageBitmap(image) | ||
| ImageLoader.loadImage(this@ShareReceiverActivity, url).into(binding.ivImage) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find ImageLoader class definition
find . -type f -name "*.kt" | xargs grep -l "class ImageLoader\|object ImageLoader" | head -20Repository: tejpratap46/AndroidVideoMotion
Length of output: 162
🏁 Script executed:
#!/bin/bash
# Read the ImageLoader.kt file to check the loadImage method signature
cat -n ./modules/imageloader/src/main/java/com/tejpratapsingh/motion/imageloader/ImageLoader.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 1503
Unnecessary coroutine launch for non-suspend Glide operation.
ImageLoader.loadImage() is a regular (non-suspend) function that returns a Glide RequestBuilder. The into() method is not a suspend function and handles threading internally. Wrapping this call in lifecycleScope.launch adds unnecessary overhead.
🔧 Suggested fix
private fun loadImage(url: String) {
- lifecycleScope.launch {
- ImageLoader.loadImage(this@ShareReceiverActivity, url).into(binding.ivImage)
- }
+ ImageLoader.loadImage(this, url).into(binding.ivImage)
}📝 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.
| private fun loadImage(url: String) { | |
| lifecycleScope.launch { | |
| val image = metadataViewModel.downloadImage(url) | |
| binding.ivImage.setImageBitmap(image) | |
| ImageLoader.loadImage(this@ShareReceiverActivity, url).into(binding.ivImage) | |
| } | |
| } | |
| private fun loadImage(url: String) { | |
| ImageLoader.loadImage(this, url).into(binding.ivImage) | |
| } |
🤖 Prompt for AI Agents
In
@modules/metadata-extractor/src/main/java/com/tejpratapsingh/motion/metadataextractor/presentation/ShareReceiverActivity.kt
around lines 119 - 123, The loadImage method wraps a non-suspending Glide call
in lifecycleScope.launch unnecessarily; remove the coroutine and call
ImageLoader.loadImage(this@ShareReceiverActivity, url).into(binding.ivImage)
directly in ShareReceiverActivity.loadImage so threading is handled by Glide
itself and avoid the extra coroutine overhead from lifecycleScope.launch.
| override fun toString(): String { | ||
| return "title:$title, description:$description, image:$image, siteName:$siteName, twitterCard:$twitterCard, url:$url, trackName:$trackName," + | ||
| "artistName:$artistName, albumName:$albumName, duration:$duration, instrument:$instrumental, plainLyrics:$plainLyrics, syncedLyrics:$syncedLyrics," + | ||
| "savedFilePath:$savedFilePath, totalLyrics:$totalLyrics, selectedLyrics:$selectedLyrics" | ||
| } |
There was a problem hiding this comment.
Fix typo in toString() output.
Line 23 contains "instrument:$instrumental" but should be "instrumental:$instrumental" to match the field name.
📝 Fix the typo
override fun toString(): String {
return "title:$title, description:$description, image:$image, siteName:$siteName, twitterCard:$twitterCard, url:$url, trackName:$trackName," +
- "artistName:$artistName, albumName:$albumName, duration:$duration, instrument:$instrumental, plainLyrics:$plainLyrics, syncedLyrics:$syncedLyrics," +
+ "artistName:$artistName, albumName:$albumName, duration:$duration, instrumental:$instrumental, plainLyrics:$plainLyrics, syncedLyrics:$syncedLyrics," +
"savedFilePath:$savedFilePath, totalLyrics:$totalLyrics, selectedLyrics:$selectedLyrics"
}📝 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 fun toString(): String { | |
| return "title:$title, description:$description, image:$image, siteName:$siteName, twitterCard:$twitterCard, url:$url, trackName:$trackName," + | |
| "artistName:$artistName, albumName:$albumName, duration:$duration, instrument:$instrumental, plainLyrics:$plainLyrics, syncedLyrics:$syncedLyrics," + | |
| "savedFilePath:$savedFilePath, totalLyrics:$totalLyrics, selectedLyrics:$selectedLyrics" | |
| } | |
| override fun toString(): String { | |
| return "title:$title, description:$description, image:$image, siteName:$siteName, twitterCard:$twitterCard, url:$url, trackName:$trackName," + | |
| "artistName:$artistName, albumName:$albumName, duration:$duration, instrumental:$instrumental, plainLyrics:$plainLyrics, syncedLyrics:$syncedLyrics," + | |
| "savedFilePath:$savedFilePath, totalLyrics:$totalLyrics, selectedLyrics:$selectedLyrics" | |
| } |
🤖 Prompt for AI Agents
In
@modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/CurrentProject.kt
around lines 21 - 25, The toString() implementation in CurrentProject.toString()
contains a typo: it emits "instrument:$instrumental" instead of the correct key
"instrumental:$instrumental"; update the string concatenation in the toString()
method to use "instrumental:$instrumental" so the output key matches the field
name and other entries.
| var songProject = CurrentProject() | ||
| private set |
There was a problem hiding this comment.
Add thread safety for shared mutable state.
The songProject property is mutable shared state that can be accessed and modified from multiple threads (e.g., Android main thread, background coroutines). Without synchronization, concurrent reads and writes can lead to race conditions and inconsistent state.
Consider using one of these approaches:
- Make
songProjectimmutable (use aMutableStateFloworStateFlowinstead) - Use
@Volatileannotation (for visibility) combined with atomic operations - Synchronize access with locks
🔒 Recommended fix using StateFlow
+import kotlinx.coroutines.flow.MutableStateFlow
+import kotlinx.coroutines.flow.StateFlow
+import kotlinx.coroutines.flow.asStateFlow
+
object ProjectManager {
private var projectDatabase: ProjectDatabase?=null
private var projectDao: ProjectDao?=null
- var songProject = CurrentProject()
- private set
+ private val _songProject = MutableStateFlow(CurrentProject())
+ val songProject: StateFlow<CurrentProject> = _songProject.asStateFlow()
+
+ fun updateSongProject(project: CurrentProject) {
+ _songProject.value = project
+ }Note: Callers would need to collect the flow or use .value to read the current project.
📝 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.
| var songProject = CurrentProject() | |
| private set | |
| import kotlinx.coroutines.flow.MutableStateFlow | |
| import kotlinx.coroutines.flow.StateFlow | |
| import kotlinx.coroutines.flow.asStateFlow | |
| object ProjectManager { | |
| private var projectDatabase: ProjectDatabase?=null | |
| private var projectDao: ProjectDao?=null | |
| private val _songProject = MutableStateFlow(CurrentProject()) | |
| val songProject: StateFlow<CurrentProject> = _songProject.asStateFlow() | |
| fun updateSongProject(project: CurrentProject) { | |
| _songProject.value = project | |
| } |
🤖 Prompt for AI Agents
In
@modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/ProjectManager.kt
around lines 15 - 16, Replace the mutable shared var songProject with a
thread-safe StateFlow: create a private MutableStateFlow initialized with
CurrentProject() (e.g., _songProject = MutableStateFlow(CurrentProject())) and
expose a public read-only StateFlow (songProject: StateFlow<CurrentProject> =
_songProject.asStateFlow()); update all writers to use _songProject.value = ...
and readers to collect or read songProject.value. This change keeps
CurrentProject immutable at the public API, ensures visibility across threads,
and preserves existing callers by providing a .value accessor for synchronous
reads.
| fun initDataBase(context: Context) { | ||
| projectDatabase = DatabaseProvider.getDatabase(context) | ||
| projectDao = projectDatabase?.projectDao() | ||
| } |
There was a problem hiding this comment.
Validate database initialization.
If DatabaseProvider.getDatabase(context) returns null, projectDao will remain null, causing saveProject() and getAllProjects() to silently fail (no-ops returning null). Consider throwing an exception or logging an error if initialization fails, so callers are aware of the problem.
💚 Proposed fix to validate initialization
fun initDataBase(context: Context) {
projectDatabase = DatabaseProvider.getDatabase(context)
projectDao = projectDatabase?.projectDao()
+ if (projectDatabase == null || projectDao == null) {
+ Log.e("ProjectManager", "Failed to initialize database")
+ throw IllegalStateException("Failed to initialize ProjectManager database")
+ }
}🤖 Prompt for AI Agents
In
@modules/ongoing/src/main/java/com/tejpratapsingh/motion/ongoing/domain/ProjectManager.kt
around lines 17 - 20, initDataBase currently assigns projectDatabase =
DatabaseProvider.getDatabase(context) and projectDao =
projectDatabase?.projectDao() but does not handle a null projectDatabase,
leading saveProject and getAllProjects to silently no-op; update initDataBase to
validate the result of DatabaseProvider.getDatabase(context) and if null either
throw an IllegalStateException (or return a boolean/Result) and log an explicit
error, or else set projectDao non-null by failing fast, so callers of
initDataBase know initialization failed and saveProject/getAllProjects won't
silently return null. Ensure you reference initDataBase,
DatabaseProvider.getDatabase, projectDatabase, projectDao, saveProject and
getAllProjects when applying the change.
1 participant
Added DashBoard screen.
Added room data base to store all the song edited songs as a project.
WIP: We need to refine the screen routing and need to work on album art caching instead of re-doing the http call .
Summary by CodeRabbit
Release Notes
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.