Add motion-video-editor module and integrate into lyrics-maker#62
Merged
Conversation
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
motion-lib | f36afd1 | Commit Preview URL Branch Preview URL |
May 25 2026, 05:03 PM |
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
sdui | f36afd1 | Commit Preview URL Branch Preview URL |
May 25 2026, 05:03 PM |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new motion-video-editor Android library with timeline models, editor/timeline Compose UIs, controlled video playback, a translucent SDUI overlay and registration, MotionTemplate SDUI provider, template updates, lyrics-maker navigation to a VideoEditor, and related tests. ChangesMotion Video Editor Module Integration
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
lyrics | f36afd1 | Commit Preview URL Branch Preview URL |
May 25 2026, 05:04 PM |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Summary
This PR adds a new motion-video-editor module with timeline UI and integrates it into the lyrics-maker navigation flow. The implementation provides a video editor screen with player controls and timeline visualization.
Critical Issues Found
Crash Risks (3): Found unhandled exceptions in MediaPlayer operations and project data parsing that will cause crashes in production. These must be fixed before merge.
Key Changes
- New motion-video-editor module with MotionEditorScreen, MotionTimeline, and TimelineUtils
- Enhanced video player with frame-stepping controls
- Updated navigation to route template selection to VideoEditor instead of ProjectDetails
The architecture is sound, but error handling for MediaPlayer lifecycle and project data parsing must be added to prevent runtime crashes.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
Comment on lines
+39
to
+44
| MotionVideoPlayerCompose( | ||
| motionVideoProducer = motionVideoProducer, | ||
| modifier = Modifier | ||
| .weight(1f) | ||
| .fillMaxWidth() | ||
| ) |
Contributor
There was a problem hiding this comment.
🛑 Crash Risk: MotionVideoPlayerCompose will crash if motionVideoProducer is null (when createFromProject fails). Add null check or conditional rendering.
Suggested change
| MotionVideoPlayerCompose( | |
| motionVideoProducer = motionVideoProducer, | |
| modifier = Modifier | |
| .weight(1f) | |
| .fillMaxWidth() | |
| ) | |
| motionVideoProducer?.let { | |
| MotionVideoPlayerCompose( | |
| motionVideoProducer = it, | |
| modifier = Modifier | |
| .weight(1f) | |
| .fillMaxWidth() | |
| ) | |
| } |
Comment on lines
+30
to
+35
| val motionVideoProducer = remember(project) { | ||
| producerFactory.createFromProject(project) | ||
| } | ||
| val timelineTracks = remember(project) { | ||
| TimelineUtils.fromSdui(context, project.sdui) | ||
| } |
Contributor
There was a problem hiding this comment.
🛑 Crash Risk: createFromProject() and fromSdui() can throw exceptions when project.sdui is empty or malformed. Wrap in error handling to prevent crashes with invalid project data.
Suggested change
| val motionVideoProducer = remember(project) { | |
| producerFactory.createFromProject(project) | |
| } | |
| val timelineTracks = remember(project) { | |
| TimelineUtils.fromSdui(context, project.sdui) | |
| } | |
| val motionVideoProducer = remember(project) { | |
| try { | |
| producerFactory.createFromProject(project) | |
| } catch (e: Exception) { | |
| null | |
| } | |
| } | |
| val timelineTracks = remember(project) { | |
| try { | |
| TimelineUtils.fromSdui(context, project.sdui) | |
| } catch (e: Exception) { | |
| emptyList() | |
| } | |
| } |
Comment on lines
+193
to
+201
| IconButton(onClick = { | ||
| currentFrame = (currentFrame - 1).coerceAtLeast(0) | ||
| isPlaying = false | ||
| }) { | ||
| Icon( | ||
| imageVector = Icons.AutoMirrored.Filled.ArrowBack, | ||
| contentDescription = "Previous Frame" | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
🛑 Crash Risk: MediaPlayer.setDataSource() and prepare() can throw IOException when file is not found or corrupted. The code at lines 108-113 (not shown in diff but nearby) needs try-catch to prevent crashes.
![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 motion-video-editor module, which includes a timeline-based editing interface and integrates it into the existing application navigation. The MotionVideoPlayerCompose component was also updated with new frame-by-frame navigation controls and a refined layout. Feedback for this PR focuses on resolving a critical issue where the timeline fails to scroll horizontally due to incorrect width measurement, replacing unstable hash codes with persistent identifiers for timeline items, and addressing potential UI overlaps in the video player controls on smaller screens.
Comment on lines
+24
to
+41
| fun MotionTimeline( | ||
| tracks: List<TimelineTrack>, | ||
| pixelsPerFrame: Float = 5f, | ||
| modifier: Modifier = Modifier | ||
| ) { | ||
| val scrollState = rememberScrollState() | ||
|
|
||
| Column( | ||
| modifier = modifier | ||
| .fillMaxWidth() | ||
| .horizontalScroll(scrollState) | ||
| .padding(vertical = 16.dp) | ||
| ) { | ||
| tracks.forEach { track -> | ||
| TimelineTrackView(track, pixelsPerFrame) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The MotionTimeline currently uses Modifier.fillMaxWidth() on its tracks, which prevents horizontal scrolling from working correctly. Because the timeline items use Modifier.offset(), they do not contribute to the measured width of the parent Box. Consequently, the horizontalScroll modifier on the Column will see a content width equal to the screen width and won't allow scrolling to items beyond the initial view.
You should calculate the total width required based on the maximum endFrame and apply it to the tracks.
Suggested change
| fun MotionTimeline( | |
| tracks: List<TimelineTrack>, | |
| pixelsPerFrame: Float = 5f, | |
| modifier: Modifier = Modifier | |
| ) { | |
| val scrollState = rememberScrollState() | |
| Column( | |
| modifier = modifier | |
| .fillMaxWidth() | |
| .horizontalScroll(scrollState) | |
| .padding(vertical = 16.dp) | |
| ) { | |
| tracks.forEach { track -> | |
| TimelineTrackView(track, pixelsPerFrame) | |
| } | |
| } | |
| } | |
| fun MotionTimeline( | |
| tracks: List<TimelineTrack>, | |
| pixelsPerFrame: Float = 5f, | |
| modifier: Modifier = Modifier | |
| ) { | |
| val scrollState = rememberScrollState() | |
| val maxFrame = tracks.flatMap { it.items }.maxOfOrNull { it.endFrame } ?: 0 | |
| val timelineWidth = (maxFrame * pixelsPerFrame).dp | |
| Column( | |
| modifier = modifier | |
| .fillMaxWidth() | |
| .horizontalScroll(scrollState) | |
| .padding(vertical = 16.dp) | |
| ) { | |
| tracks.forEach { track -> | |
| TimelineTrackView(track, pixelsPerFrame, timelineWidth) | |
| } | |
| } | |
| } |
Comment on lines
+44
to
+51
| fun TimelineTrackView( | ||
| track: TimelineTrack, | ||
| pixelsPerFrame: Float | ||
| ) { | ||
| Box( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .height(50.dp) |
There was a problem hiding this comment.
Update TimelineTrackView to accept and apply the calculated width to ensure the scrollable area is correctly defined.
Suggested change
| fun TimelineTrackView( | |
| track: TimelineTrack, | |
| pixelsPerFrame: Float | |
| ) { | |
| Box( | |
| modifier = Modifier | |
| .fillMaxWidth() | |
| .height(50.dp) | |
| fun TimelineTrackView( | |
| track: TimelineTrack, | |
| pixelsPerFrame: Float, | |
| width: androidx.compose.ui.unit.Dp | |
| ) { | |
| Box( | |
| modifier = Modifier | |
| .width(width) | |
| .height(50.dp) |
Comment on lines
185
to
233
| Box( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| horizontalArrangement = Arrangement.SpaceBetween | ||
| contentAlignment = Alignment.Center | ||
| ) { | ||
| IconButton(onClick = { isPlaying = !isPlaying }) { | ||
| Icon( | ||
| imageVector = if (isPlaying) Icons.Default.Pause else Icons.Default.PlayArrow, | ||
| contentDescription = if (isPlaying) "Pause" else "Play" | ||
| ) | ||
| Row( | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| horizontalArrangement = Arrangement.spacedBy(8.dp) | ||
| ) { | ||
| IconButton(onClick = { | ||
| currentFrame = (currentFrame - 1).coerceAtLeast(0) | ||
| isPlaying = false | ||
| }) { | ||
| Icon( | ||
| imageVector = Icons.AutoMirrored.Filled.ArrowBack, | ||
| contentDescription = "Previous Frame" | ||
| ) | ||
| } | ||
|
|
||
| IconButton( | ||
| onClick = { isPlaying = !isPlaying }, | ||
| modifier = Modifier.background( | ||
| color = MaterialTheme.colorScheme.primary, | ||
| shape = RoundedCornerShape(8.dp) | ||
| ) | ||
| ) { | ||
| Icon( | ||
| imageVector = if (isPlaying) Icons.Default.Pause else Icons.Default.PlayArrow, | ||
| contentDescription = if (isPlaying) "Pause" else "Play", | ||
| tint = MaterialTheme.colorScheme.onPrimary | ||
| ) | ||
| } | ||
|
|
||
| IconButton(onClick = { | ||
| currentFrame = (currentFrame + 1).coerceAtMost(totalFrames) | ||
| isPlaying = false | ||
| }) { | ||
| Icon( | ||
| imageVector = Icons.AutoMirrored.Filled.ArrowForward, | ||
| contentDescription = "Next Frame" | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| Text( | ||
| text = "${formatTime(currentFrame, motionConfig.fps)} / ${formatTime(totalFrames, motionConfig.fps)}", | ||
| style = MaterialTheme.typography.bodyMedium | ||
| style = MaterialTheme.typography.bodyMedium, | ||
| modifier = Modifier.align(Alignment.CenterEnd) | ||
| ) | ||
| } |
There was a problem hiding this comment.
The use of a Box with centered controls and an end-aligned timestamp (Alignment.CenterEnd) can lead to overlapping UI elements on smaller screens. Consider using a Row with weighted spacers or a layout that explicitly prevents overlap while maintaining the desired alignment.
| id = "track_$index", | ||
| items = listOf( | ||
| TimelineItem( | ||
| id = view.hashCode().toString(), |
There was a problem hiding this comment.
Using view.hashCode().toString() as an identifier for TimelineItem is unreliable. Hash codes are not guaranteed to be stable across object recreations or process restarts, which can lead to issues with Compose's internal state tracking and animations. It is recommended to use a stable unique ID from the data source if available.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/utils/TimelineUtils.kt (1)
24-24: ⚡ Quick winUse deterministic IDs instead of
hashCode()for timeline items.Line 24 uses a non-stable identity source. Prefer a deterministic value derived from track/index and frame range to avoid collisions and state mismatch.
Proposed fix
TimelineItem( - id = view.hashCode().toString(), + id = "item_${index}_${view.javaClass.simpleName}_${view.startFrame}_${view.endFrame}", type = view.javaClass.simpleName, startFrame = view.startFrame, endFrame = view.endFrame, label = view.javaClass.simpleName )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/utils/TimelineUtils.kt` at line 24, Replace the non-deterministic id assignment "id = view.hashCode().toString()" with a stable identifier derived from item metadata (e.g., track id/index and frame range) so IDs don't change across renders; locate the timeline item creation where "id = view.hashCode().toString()" is set (in TimelineUtils.kt) and build a deterministic string using the track identifier plus clip/index and start/end frame values (for example combine trackIndex, clipIndex, startFrame, endFrame) so each timeline item has a consistent, collision-resistant id.modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.kt (1)
28-35: ⚡ Quick winAdd error handling for producer and timeline creation.
The calls to
producerFactory.createFromProject()andTimelineUtils.fromSdui()could throw exceptions if the project data or SDUI structure is malformed. Consider wrapping these in try-catch blocks and displaying an error state composable when creation fails.🛡️ Example error handling pattern
+ var errorMessage by remember { mutableStateOf<String?>(null) } + val motionVideoProducer = remember(project) { - producerFactory.createFromProject(project) + try { + producerFactory.createFromProject(project) + } catch (e: Exception) { + errorMessage = "Failed to load project: ${e.message}" + null + } } val timelineTracks = remember(project) { - TimelineUtils.fromSdui(context, project.sdui) + try { + TimelineUtils.fromSdui(context, project.sdui) + } catch (e: Exception) { + errorMessage = "Failed to parse timeline: ${e.message}" + emptyList() + } } + if (errorMessage != null) { + // Show error UI + Box(modifier = modifier.fillMaxSize(), contentAlignment = Alignment.Center) { + Text(errorMessage!!, color = MaterialTheme.colorScheme.error) + } + return + } + + if (motionVideoProducer == null) { + return + } + Column(modifier = modifier.fillMaxSize()) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.kt` around lines 28 - 35, Wrap the potentially-throwing calls to SDUIMotionVideoProducerFactory.createFromProject(project) and TimelineUtils.fromSdui(context, project.sdui) in try-catch blocks inside MotionEditorScreen and store successes or caught exceptions in remembered state (e.g., remember { mutableStateOf(...) } for producer, timeline, and an error object); if an exception is caught, set the error state and render an error-state composable instead of the normal UI so the exception doesn’t escape Compose and the user sees a clear failure message.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/AppNavHost.kt (1)
150-161: ⚡ Quick winConsider handling the case when project is not found.
When
projectIdis null or the project cannot be found in the list, the composable renders nothing, resulting in a blank screen. Consider adding a fallback UI (error message or loading indicator) or navigating back to improve the user experience.💡 Example error handling
composable(route = Screen.VideoEditor.route) { backStackEntry -> val projectId = backStackEntry.arguments?.getString("projectId") val projects = projectsViewModel.projects.collectAsStateWithLifecycle() val project = projects.value.find { it.id == projectId } - project?.let { + if (project != null) { MotionEditorScreen( - project = it, + project = project, modifier = modifier, ) + } else { + Box(modifier = modifier.fillMaxSize(), contentAlignment = Alignment.Center) { + Text("Project not found", style = MaterialTheme.typography.bodyLarge) + } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/AppNavHost.kt` around lines 150 - 161, The VideoEditor composable currently renders nothing when projectId is null or projectsViewModel.projects can't find a matching project (project variable), causing a blank screen; inside the composable for Screen.VideoEditor.route, add explicit handling for a missing projectId or missing project: either show a small fallback UI (error message, "Project not found" and a retry/loading state) or programmatically navigate back (use the NavController to popBackStack or navigate to a safe screen) instead of silently doing nothing; update the block that finds project (projectsViewModel.projects.collectAsStateWithLifecycle() and project lookup) and replace the project?.let { MotionEditorScreen(...) } with logic that shows MotionEditorScreen when project exists and otherwise shows the fallback UI or triggers navigation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/motion-video-editor/build.gradle`:
- Around line 1-4: The plugins block is missing the Android Kotlin plugin so
Kotlin sources won't compile; update the plugins block that currently contains
alias(libs.plugins.android.library) and alias(libs.plugins.kotlin.compose) to
also apply the Kotlin Android plugin (org.jetbrains.kotlin.android) — if you use
the version catalog, add alias(libs.plugins.kotlin.android) or otherwise add the
'kotlin.android' plugin identifier so the Android library module (plugins block)
properly configures Kotlin compilation for the files in this module.
In `@modules/motion-video-editor/src/main/AndroidManifest.xml`:
- Around line 2-3: Remove the deprecated package attribute from the root
<manifest> tag in AndroidManifest.xml: delete the
package="com.tejpratapsingh.motioneditor" attribute while preserving the
xmlns:android="http://schemas.android.com/apk/res/android" declaration; verify
the appId/namespace is defined in your Gradle configuration (so the manifest
relies on the build.gradle namespace) and ensure no code relies on reading the
removed attribute from the manifest at runtime.
In
`@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt`:
- Around line 31-35: The Column currently uses fillMaxWidth() which prevents
horizontalScroll from working because offset() only repositions content after
layout; remove fillMaxWidth() from the track container and compute the maximum
timeline width from your item data (e.g., iterate items to find max end/time)
and then apply that explicit width to each track composable instead of
fillMaxWidth(); update the track layout or modifier (the places around Column,
horizontalScroll(scrollState), fillMaxWidth(), and offset()) to use
Modifier.width(maxTimelineWidth) so the scrollable area matches the true
timeline content width.
- Around line 66-67: The current conversion of pixel values to dp in the
MotionTimeline code uses .dp directly on raw pixel numbers (startPx and widthPx)
and doesn't guard against negative durations; replace the .dp conversions with
density-aware conversions using LocalDensity.current.toDp(...) on the pixel
floats (compute pixel floats from item.startFrame * pixelsPerFrame and
(item.endFrame - item.startFrame) * pixelsPerFrame), and ensure the computed
duration uses coerceAtLeast(0) on (item.endFrame - item.startFrame) before
converting so widthPx can never be negative.
In
`@modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt`:
- Around line 185-233: The centered control Row and the right-aligned Text
inside Box can overlap on small screens; replace the Box layout with a single
Row (or a Row inside the Box) using horizontal Arrangement.SpaceBetween and
three slots: left Spacer, centered control Row (the IconButton group that
updates currentFrame/isPlaying), and right-aligned Text (the
"${formatTime(currentFrame, motionConfig.fps)} / ${formatTime(totalFrames,
motionConfig.fps)}"), or alternatively give the center control Row a fixed
minWidth and the Text a weight or a Spacer with minWidth to guarantee separation
so the IconButton group and the time Text never overlap on narrow/large-font
displays.
---
Nitpick comments:
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/AppNavHost.kt`:
- Around line 150-161: The VideoEditor composable currently renders nothing when
projectId is null or projectsViewModel.projects can't find a matching project
(project variable), causing a blank screen; inside the composable for
Screen.VideoEditor.route, add explicit handling for a missing projectId or
missing project: either show a small fallback UI (error message, "Project not
found" and a retry/loading state) or programmatically navigate back (use the
NavController to popBackStack or navigate to a safe screen) instead of silently
doing nothing; update the block that finds project
(projectsViewModel.projects.collectAsStateWithLifecycle() and project lookup)
and replace the project?.let { MotionEditorScreen(...) } with logic that shows
MotionEditorScreen when project exists and otherwise shows the fallback UI or
triggers navigation.
In
`@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.kt`:
- Around line 28-35: Wrap the potentially-throwing calls to
SDUIMotionVideoProducerFactory.createFromProject(project) and
TimelineUtils.fromSdui(context, project.sdui) in try-catch blocks inside
MotionEditorScreen and store successes or caught exceptions in remembered state
(e.g., remember { mutableStateOf(...) } for producer, timeline, and an error
object); if an exception is caught, set the error state and render an
error-state composable instead of the normal UI so the exception doesn’t escape
Compose and the user sees a clear failure message.
In
`@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/utils/TimelineUtils.kt`:
- Line 24: Replace the non-deterministic id assignment "id =
view.hashCode().toString()" with a stable identifier derived from item metadata
(e.g., track id/index and frame range) so IDs don't change across renders;
locate the timeline item creation where "id = view.hashCode().toString()" is set
(in TimelineUtils.kt) and build a deterministic string using the track
identifier plus clip/index and start/end frame values (for example combine
trackIndex, clipIndex, startFrame, endFrame) so each timeline item has a
consistent, collision-resistant id.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea70eeef-d5cd-4c10-afc7-88ba7ebb2e9b
📒 Files selected for processing (12)
.idea/gradle.xmlgradle/libs.versions.tomlmodules/lyrics-maker/build.gradlemodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/AppNavHost.ktmodules/motion-video-editor/build.gradlemodules/motion-video-editor/src/main/AndroidManifest.xmlmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/TimelineData.ktmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.ktmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.ktmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/utils/TimelineUtils.ktmodules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.ktsettings.gradle
Comment on lines
+1
to
+4
| plugins { | ||
| alias(libs.plugins.android.library) | ||
| alias(libs.plugins.kotlin.compose) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking where Kotlin Android plugin is applied..."
rg -n --iglob 'build.gradle*' 'org\.jetbrains\.kotlin\.android|libs\.plugins\.kotlin\.android|kotlin-android'
echo
echo "Checking for convention-wide application blocks..."
rg -n --iglob 'build.gradle*' 'subprojects|allprojects|apply plugin:'Repository: tejpratap46/AndroidVideoMotion
Length of output: 125
🏁 Script executed:
# Read the actual build.gradle file in the module
cat -n modules/motion-video-editor/build.gradle 2>/dev/null || cat -n modules/motion-video-editor/build.gradle.kts 2>/dev/null || echo "File not found in expected locations"Repository: tejpratap46/AndroidVideoMotion
Length of output: 2053
🏁 Script executed:
# Check for buildSrc or convention plugins
find . -type d -name "buildSrc" -o -name "build-logic" | head -5
ls -la gradle/ 2>/dev/null || echo "No gradle directory"Repository: tejpratap46/AndroidVideoMotion
Length of output: 315
🏁 Script executed:
# Check if kotlin.compose is defined and what it maps to
rg -n 'kotlin\.compose|kotlin-compose' gradle/ build.gradle* settings.gradle* gradle.properties 2>/dev/null || echo "Not found in root configs"Repository: tejpratap46/AndroidVideoMotion
Length of output: 199
🏁 Script executed:
# Verify Kotlin sources actually exist in the module
find modules/motion-video-editor -type f \( -name "*.kt" -o -name "*.kts" \) | head -10Repository: tejpratap46/AndroidVideoMotion
Length of output: 455
Apply kotlin.android in this module.
The module has Kotlin sources (4 .kt files) and is configured as an Android library, but the org.jetbrains.kotlin.android plugin is missing from the plugins block. The kotlin.compose plugin alone (which provides the Kotlin Compiler Plugin for Compose) does not replace the need for the Android Kotlin plugin. Without it, Kotlin compilation setup for Android may not be properly configured.
Proposed fix
plugins {
alias(libs.plugins.android.library)
+ alias(libs.plugins.kotlin.android)
alias(libs.plugins.kotlin.compose)
}📝 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.
Suggested change
| plugins { | |
| alias(libs.plugins.android.library) | |
| alias(libs.plugins.kotlin.compose) | |
| } | |
| plugins { | |
| alias(libs.plugins.android.library) | |
| alias(libs.plugins.kotlin.android) | |
| alias(libs.plugins.kotlin.compose) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/motion-video-editor/build.gradle` around lines 1 - 4, The plugins
block is missing the Android Kotlin plugin so Kotlin sources won't compile;
update the plugins block that currently contains
alias(libs.plugins.android.library) and alias(libs.plugins.kotlin.compose) to
also apply the Kotlin Android plugin (org.jetbrains.kotlin.android) — if you use
the version catalog, add alias(libs.plugins.kotlin.android) or otherwise add the
'kotlin.android' plugin identifier so the Android library module (plugins block)
properly configures Kotlin compilation for the files in this module.
Comment on lines
+31
to
+35
| Column( | ||
| modifier = modifier | ||
| .fillMaxWidth() | ||
| .horizontalScroll(scrollState) | ||
| .padding(vertical = 16.dp) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "MotionTimeline.kt" -type fRepository: tejpratap46/AndroidVideoMotion
Length of output: 171
🏁 Script executed:
cat -n ./modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 4091
🏁 Script executed:
find . -name "TimelineItem.kt" -o -name "TimelineTrack.kt" | head -5Repository: tejpratap46/AndroidVideoMotion
Length of output: 56
🏁 Script executed:
rg "class TimelineItem|data class TimelineItem|class TimelineTrack|data class TimelineTrack" -A 3Repository: tejpratap46/AndroidVideoMotion
Length of output: 971
Remove fillMaxWidth() from tracks to enable horizontal scrolling based on timeline content width.
When tracks use fillMaxWidth() (line 50) with items positioned via offset() (line 71), the offset modifier doesn't expand measured content width—it only repositions after layout. This leaves zero scrollable range, making off-screen items unreachable.
Compute the maximum timeline width from items and apply it explicitly to each track:
Proposed fix
`@Composable`
fun MotionTimeline(
tracks: List<TimelineTrack>,
pixelsPerFrame: Float = 5f,
modifier: Modifier = Modifier
) {
val scrollState = rememberScrollState()
+ val maxEndFrame = tracks.flatMap { it.items }.maxOfOrNull { it.endFrame } ?: 0
+ val timelineWidthDp = (maxEndFrame * pixelsPerFrame).dp
Column(
modifier = modifier
- .fillMaxWidth()
.horizontalScroll(scrollState)
.padding(vertical = 16.dp)
) {
tracks.forEach { track ->
- TimelineTrackView(track, pixelsPerFrame)
+ TimelineTrackView(
+ track = track,
+ pixelsPerFrame = pixelsPerFrame,
+ modifier = Modifier.width(timelineWidthDp)
+ )
}
}
}
`@Composable`
fun TimelineTrackView(
track: TimelineTrack,
- pixelsPerFrame: Float
+ pixelsPerFrame: Float,
+ modifier: Modifier = Modifier
) {
Box(
- modifier = Modifier
- .fillMaxWidth()
+ modifier = modifier
.height(50.dp)
.padding(vertical = 4.dp)
.background(Color.DarkGray.copy(alpha = 0.1f))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt`
around lines 31 - 35, The Column currently uses fillMaxWidth() which prevents
horizontalScroll from working because offset() only repositions content after
layout; remove fillMaxWidth() from the track container and compute the maximum
timeline width from your item data (e.g., iterate items to find max end/time)
and then apply that explicit width to each track composable instead of
fillMaxWidth(); update the track layout or modifier (the places around Column,
horizontalScroll(scrollState), fillMaxWidth(), and offset()) to use
Modifier.width(maxTimelineWidth) so the scrollable area matches the true
timeline content width.
Comment on lines
+66
to
+67
| val startPx = (item.startFrame * pixelsPerFrame).dp | ||
| val widthPx = ((item.endFrame - item.startFrame) * pixelsPerFrame).dp |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt | head -100Repository: tejpratap46/AndroidVideoMotion
Length of output: 3727
🏁 Script executed:
find . -name "*.kt" | xargs grep -l "class TimelineItem"Repository: tejpratap46/AndroidVideoMotion
Length of output: 166
🏁 Script executed:
cat -n ./modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/TimelineData.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 435
🏁 Script executed:
rg "TimelineItem\(" -A 2 | head -50Repository: tejpratap46/AndroidVideoMotion
Length of output: 1990
🏁 Script executed:
cat -n ./modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/utils/TimelineUtils.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 1600
🏁 Script executed:
find . -name "*.kt" | xargs grep -l "class MotionView"Repository: tejpratap46/AndroidVideoMotion
Length of output: 160
🏁 Script executed:
cat -n ./modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionViewParser.kt | head -150Repository: tejpratap46/AndroidVideoMotion
Length of output: 5677
🏁 Script executed:
rg "pixelsPerFrame" -B 2 -A 2Repository: tejpratap46/AndroidVideoMotion
Length of output: 3397
🌐 Web query:
Compose width modifier negative dimension behavior
💡 Result:
In Jetpack Compose, using negative values with the width-related modifiers does not produce a defined “negative width” layout. Instead, the modifier’s width value becomes part of the incoming layout constraints, and the layout system treats negative/invalid size results as empty/zero (or clamps/overrides via constraint handling). If you need to visually “shrink” or “invert” width, you generally shouldn’t use a negative width modifier; use an approach like offset for positioning and/or use layout to adjust measured constraints. 1) What Modifier.width does (and why negative values don’t behave as “negative width”) - Modifier.width(width: Dp) sets the content’s preferred width by setting both minWidth and maxWidth constraints to the passed Dp value. The doc also notes that incoming Constraints may override the value, forcing the content to be smaller or larger [1]. - Compose layout is constraint-driven: Constraints define minimum/maximum bounds for width/height, and measured size must fall within those bounds [2]. So, if you pass a negative Dp to Modifier.width, you’re asking for constraints with a negative bound. The Compose measurement/layout pipeline doesn’t have a normal meaning for negative physical area; effectively, you end up with an empty/degenerate size result. 2) Compose geometry treats negative areas as empty - In Compose UI geometry, “Whether this size encloses a non-zero area” and it explicitly states: “Negative areas are considered empty.” [3] This is consistent with the idea that even if width calculations go negative, the resulting rendered area is empty. 3) Use offset for “negative dimension-like” behavior (positioning without changing measurements) - Compose’s offset modifier can be negative, and importantly: “adding an offset to a composable does not change its measurements” [4]. - For example, if the goal of a “negative width” is to move content to the left/right while keeping its normal measurement, offset is the intended primitive. 4) If you truly need to control measured width beyond the parent’s constraints, use required* / custom layout / constraint manipulation - Docs explain that requiredSize/requiredWidth exist for cases where you need to override incoming constraints rather than adapt to them [2]. Practical guidance - Don’t rely on negative width/size values to get an “inverted” or “negative dimension” layout. - Prefer: a) offset(x = negativeDp) to reposition without affecting measurement [4], and/or b) custom layout (Modifier.layout / custom MeasurePolicy) if you need to compute your own constraints and placement. If you tell me which Compose modifier you used (width vs size vs widthIn vs requiredWidth) and your target behavior (e.g., “visual crop from the right”, “mirror horizontally”, “allocate space but draw backwards”), I can suggest the most idiomatic modifier chain.
Citations:
- 1: https://developer.android.com/reference/kotlin/androidx/compose/foundation/layout/width.modifier
- 2: https://developer.android.com/develop/ui/compose/layouts/constraints-modifiers
- 3: https://composables.com/docs/androidx.compose.ui/ui-geometry/classes/Size
- 4: https://developer.android.com/develop/ui/compose/modifiers
🌐 Web query:
Compose LocalDensity toDp conversion pixels to dp density aware
💡 Result:
In Jetpack Compose, do conversions inside a composable using LocalDensity.current (density-aware). For pixels to dp, use toDp on a pixel value: with(LocalDensity.current) { px.toDp } [1]. Example (pixels -> dp): @Composable fun pxToDp(px: Int): Dp = with(LocalDensity.current) { px.toDp } For dp <-> px density math (what Compose is doing under the hood): dp is a density-independent unit and the Android docs state px = dp * (dpi / 160); the Density/DisplayMetrics conversion is based on the current screen density [2]. Notes: - LocalDensity provides the Density implementation used to transform between dp (and sp) and pixel units [3]. - You typically convert in layout/draw modifiers where density is available, e.g., with(LocalDensity.current) {... } [3].
Citations:
- 1: https://stackoverflow.com/questions/65921799/how-to-convert-dp-to-pixels-in-android-jetpack-compose
- 2: https://developer.android.com/training/multiscreen/screendensities?hl=en
- 3: https://composables.com/jetpack-compose/androidx.compose.ui/ui/properties/LocalDensity/api
Fix density-unsafe px-to-dp conversion and guard against negative duration.
Lines 66–67 directly apply .dp to pixel values without accounting for device density. Use LocalDensity.current.toDp() for proper density-aware conversion. Additionally, missing validation allows endFrame < startFrame, producing negative width values; apply coerceAtLeast(0) to the duration.
Proposed fix (density-safe conversion + non-negative duration)
+import androidx.compose.ui.platform.LocalDensity
@@
fun TimelineItemView(
item: TimelineItem,
pixelsPerFrame: Float
) {
- val startPx = (item.startFrame * pixelsPerFrame).dp
- val widthPx = ((item.endFrame - item.startFrame) * pixelsPerFrame).dp
+ val density = LocalDensity.current
+ val durationFrames = (item.endFrame - item.startFrame).coerceAtLeast(0)
+ val startDp = with(density) { (item.startFrame * pixelsPerFrame).toDp() }
+ val widthDp = with(density) { (durationFrames * pixelsPerFrame).toDp() }
@@
modifier = Modifier
- .offset(x = startPx)
- .width(widthPx)
+ .offset(x = startDp)
+ .width(widthDp)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt`
around lines 66 - 67, The current conversion of pixel values to dp in the
MotionTimeline code uses .dp directly on raw pixel numbers (startPx and widthPx)
and doesn't guard against negative durations; replace the .dp conversions with
density-aware conversions using LocalDensity.current.toDp(...) on the pixel
floats (compute pixel floats from item.startFrame * pixelsPerFrame and
(item.endFrame - item.startFrame) * pixelsPerFrame), and ensure the computed
duration uses coerceAtLeast(0) on (item.endFrame - item.startFrame) before
converting so widthPx can never be negative.
tejpratap46
force-pushed
the
feat/motion-video-editor
branch
from
933ef47 to
6f7536c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayer.kt (1)
157-162:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
MediaPlayerinitialization failures to prevent crashes and resource leaks.Unguarded
setDataSource()andprepare()calls can throwIOException(file not found, unsupported format) orIllegalStateException, crashing the view. Additionally,prepare()blocks the main thread—useprepareAsync()to prevent ANR.Wrap initialization in try-catch, release the player on failure, and only add to
activePlayersafter successful startup:Proposed fix
- val mediaPlayer = - MediaPlayer().apply { - setDataSource(audio.file.absolutePath) - prepare() - start() - } - activePlayers[audio] = mediaPlayer + val mediaPlayer = MediaPlayer() + try { + mediaPlayer.setDataSource(audio.file.absolutePath) + mediaPlayer.prepareAsync() + mediaPlayer.start() + activePlayers[audio] = mediaPlayer + } catch (e: Exception) { + mediaPlayer.release() + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayer.kt` around lines 157 - 162, The MediaPlayer initialization must be made fail-safe: wrap the MediaPlayer creation and setup (the MediaPlayer() instance, setDataSource(audio.file.absolutePath), and prepare/start) in a try/catch to catch IOException and IllegalStateException, call mediaPlayer.release() on any failure to avoid leaks, and only add the instance to activePlayers after successful startup; moreover replace blocking prepare() with prepareAsync() and move start() to the onPreparedListener to avoid blocking the main thread and to ensure start() is only called after successful prepare.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.kt`:
- Around line 23-35: The current mapping of motionProject.metadata["lyrics"] to
SyncedLyricFrame uses unchecked Gson casts and can throw on malformed entries;
replace the .map with .mapNotNull and, inside the transform for each item, first
ensure the item.isJsonObject, then safely get the "frame" and "text" properties
only if they are the expected primitive/number/string types (check
isJsonPrimitive and isNumber or use isJsonPrimitive before asInt/asString),
constructing and returning a SyncedLyricFrame only for valid entries and
returning null to skip invalid ones so producer creation no longer crashes.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/AccentLyricsTemplate.kt`:
- Around line 87-91: In the preview overlay inside AccentLyricsTemplate's
preview rendering, the translucentMotionView is currently using
lyrics.last().frame for endFrame which can extend playback; update the
translucentMotionView call to use the preview's frame range (use
preview.endFrame for endFrame — and if startFrame is also meant to be the
preview start, use preview.startFrame instead of lyrics.first().frame) so the
overlay matches the preview snippet.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/ZoomLyricsTemplate.kt`:
- Around line 86-90: The translucentMotionView call is using lyrics.last().frame
as the overlay end, extending the preview; change the endFrame argument to the
preview's actual endFrame variable (use endFrame instead of lyrics.last().frame)
within ZoomLyricsTemplate so the overlay aligns with the preview end; update the
translucentMotionView invocation (startFrame = lyrics.first().frame, endFrame =
endFrame) and ensure the preview endFrame symbol is in scope where this call
occurs.
In
`@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.kt`:
- Line 54: The currentFrame state is remembered unconditionally so it persists
across different projects; update its creation to depend on the current project
so it resets when project changes by replacing the line that declares
currentFrame (var currentFrame by remember { mutableIntStateOf(0) }) with a
remember keyed on the project (e.g., var currentFrame by remember(project) {
mutableIntStateOf(0) }) in MotionEditorScreen (so the state is recreated
whenever project changes).
In
`@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt`:
- Around line 166-170: formatFrameToTime currently divides by fps without
validation which can cause a divide-by-zero; update the function
(formatFrameToTime) to guard against fps <= 0—if fps is <= 0, return a safe
default string like "00:00" (or alternatively throw an IllegalArgumentException)
instead of performing the division; ensure all calculations (totalSeconds,
minutes, seconds) only run when fps > 0 so the function never divides by zero.
In
`@modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt`:
- Around line 80-86: The playback loop inside the LaunchedEffect for
effectiveIsPlaying snapshots effectiveCurrentFrame into currentFrameTracker once
and never re-syncs, causing desync when the parent updates currentFrame; update
the loop in MotionVideoPlayerCompose (the LaunchedEffect that uses
effectiveIsPlaying, frameDurationMs, lastFrameTime and currentFrameTracker) to
re-read effectiveCurrentFrame on each iteration (or remove the one-time snapshot
and use effectiveCurrentFrame directly) so the playing progression follows
external seeks/updates; ensure you still compute timing with frameDurationMs and
update lastFrameTime appropriately while respecting the externally-driven
effectiveCurrentFrame.
In
`@modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/sdui/MotionTemplateSDUIProvider.kt`:
- Around line 34-65: The code sets a global config via
setCurrentConfig(motionConfig) and never resets it, leading to leaked shared
state; to fix, capture the previous config (e.g., val previous =
provideCurrentConfig()) before calling setCurrentConfig(motionConfig), run the
SDUI generation (MotionVideoProducer.with(...), template.buildContent(...),
createMotionSDUIJson(...)) inside a try block, and restore the previous config
in a finally block via setCurrentConfig(previous) so the global config is always
restored even on errors.
---
Outside diff comments:
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayer.kt`:
- Around line 157-162: The MediaPlayer initialization must be made fail-safe:
wrap the MediaPlayer creation and setup (the MediaPlayer() instance,
setDataSource(audio.file.absolutePath), and prepare/start) in a try/catch to
catch IOException and IllegalStateException, call mediaPlayer.release() on any
failure to avoid leaks, and only add the instance to activePlayers after
successful startup; moreover replace blocking prepare() with prepareAsync() and
move start() to the onPreparedListener to avoid blocking the main thread and to
ensure start() is only called after successful prepare.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39f5078e-068b-4d69-940e-8beae83f21a3
📒 Files selected for processing (27)
.idea/gradle.xmlgradle/libs.versions.tomlmodules/lyrics-maker/build.gradlemodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/AppNavHost.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/GradientTextCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/LyricsTemplateSelector.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectsScreenCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/AccentLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/RainbowLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/ZoomLyricsTemplate.ktmodules/motion-video-editor/build.gradlemodules/motion-video-editor/src/main/AndroidManifest.xmlmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/TimelineData.ktmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.ktmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.ktmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/utils/TimelineUtils.ktmodules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/background/TranslucentMotionView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayer.ktmodules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionSduiInitializer.ktmodules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/MotionSduiRegistryTest.ktmodules/templates/src/androidTest/java/com/tejpratapsingh/motionlib/templates/sdui/MotionTemplateSDUIProviderTest.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/BackgroundExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/sdui/MotionTemplateSDUIProvider.ktsettings.gradle
✅ Files skipped from review due to trivial changes (3)
- modules/motion-video-editor/src/main/AndroidManifest.xml
- modules/templates/src/androidTest/java/com/tejpratapsingh/motionlib/templates/sdui/MotionTemplateSDUIProviderTest.kt
- modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/TimelineData.kt
🚧 Files skipped from review as they are similar to previous changes (6)
- .idea/gradle.xml
- gradle/libs.versions.toml
- modules/lyrics-maker/build.gradle
- modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/utils/TimelineUtils.kt
- modules/motion-video-editor/build.gradle
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/AppNavHost.kt
Comment on lines
+23
to
+35
| motionProject.metadata.get("lyrics")?.takeIf { it.isJsonArray }?.asJsonArray?.map { | ||
| SyncedLyricFrame( | ||
| frame = | ||
| it.asJsonObject | ||
| .get("frame") | ||
| ?.takeIf { f -> f.isJsonPrimitive } | ||
| ?.asInt ?: 0, | ||
| text = | ||
| it.asJsonObject | ||
| .get("text") | ||
| ?.takeIf { t -> t.isJsonPrimitive } | ||
| ?.asString ?: "", | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the current parsing block uses unchecked Gson casts/conversions.
rg -n -C3 'asJsonObject|asInt' modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 626
🏁 Script executed:
# Read the full file to understand context
wc -l modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 192
🏁 Script executed:
# Read the complete file
cat -n modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 6273
🏁 Script executed:
# Search for tests related to this file or MultiLyricsVideoProducer
fd -i 'multilyricsvideoproducer.*test' modules/lyrics-maker/Repository: tejpratap46/AndroidVideoMotion
Length of output: 56
🏁 Script executed:
# Check if there are similar Gson parsing patterns elsewhere in the codebase
rg -n 'asJsonObject|asJsonArray' modules/lyrics-maker/src --type kotlin -A2 -B2 | head -50Repository: tejpratap46/AndroidVideoMotion
Length of output: 4135
Guard malformed lyrics entries before unsafe Gson casts.
Lines 26, 29, and 31 use unchecked Gson conversions that will throw exceptions if metadata contains non-object items or non-numeric primitives. Replace .map with .mapNotNull and add type checks (isJsonObject, isNumber) before calling asJsonObject() and asInt to gracefully skip invalid entries instead of crashing producer creation.
Proposed fix
- val lyrics =
- motionProject.metadata.get("lyrics")?.takeIf { it.isJsonArray }?.asJsonArray?.map {
- SyncedLyricFrame(
- frame =
- it.asJsonObject
- .get("frame")
- ?.takeIf { f -> f.isJsonPrimitive }
- ?.asInt ?: 0,
- text =
- it.asJsonObject
- .get("text")
- ?.takeIf { t -> t.isJsonPrimitive }
- ?.asString ?: "",
- )
- } ?: emptyList()
+ val lyrics =
+ motionProject.metadata.get("lyrics")
+ ?.takeIf { it.isJsonArray }
+ ?.asJsonArray
+ ?.mapNotNull { element ->
+ val obj = element.takeIf { it.isJsonObject }?.asJsonObject ?: return@mapNotNull null
+ SyncedLyricFrame(
+ frame =
+ obj.get("frame")
+ ?.takeIf { f -> f.isJsonPrimitive && f.asJsonPrimitive.isNumber }
+ ?.asInt ?: 0,
+ text =
+ obj.get("text")
+ ?.takeIf { t -> t.isJsonPrimitive }
+ ?.asString ?: "",
+ )
+ } ?: emptyList()📝 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.
Suggested change
| motionProject.metadata.get("lyrics")?.takeIf { it.isJsonArray }?.asJsonArray?.map { | |
| SyncedLyricFrame( | |
| frame = | |
| it.asJsonObject | |
| .get("frame") | |
| ?.takeIf { f -> f.isJsonPrimitive } | |
| ?.asInt ?: 0, | |
| text = | |
| it.asJsonObject | |
| .get("text") | |
| ?.takeIf { t -> t.isJsonPrimitive } | |
| ?.asString ?: "", | |
| ) | |
| val lyrics = | |
| motionProject.metadata.get("lyrics") | |
| ?.takeIf { it.isJsonArray } | |
| ?.asJsonArray | |
| ?.mapNotNull { element -> | |
| val obj = element.takeIf { it.isJsonObject }?.asJsonObject ?: return@mapNotNull null | |
| SyncedLyricFrame( | |
| frame = | |
| obj.get("frame") | |
| ?.takeIf { f -> f.isJsonPrimitive && f.asJsonPrimitive.isNumber } | |
| ?.asInt ?: 0, | |
| text = | |
| obj.get("text") | |
| ?.takeIf { t -> t.isJsonPrimitive } | |
| ?.asString ?: "", | |
| ) | |
| } ?: emptyList() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.kt`
around lines 23 - 35, The current mapping of motionProject.metadata["lyrics"] to
SyncedLyricFrame uses unchecked Gson casts and can throw on malformed entries;
replace the .map with .mapNotNull and, inside the transform for each item, first
ensure the item.isJsonObject, then safely get the "frame" and "text" properties
only if they are the expected primitive/number/string types (check
isJsonPrimitive and isNumber or use isJsonPrimitive before asInt/asString),
constructing and returning a SyncedLyricFrame only for valid entries and
returning null to skip invalid ones so producer creation no longer crashes.
Comment on lines
+87
to
+91
| translucentMotionView( | ||
| color = "#28000000", | ||
| startFrame = lyrics.first().frame, | ||
| endFrame = lyrics.last().frame, | ||
| ) |
Contributor
There was a problem hiding this comment.
Preview overlay should use the preview frame range.
In preview, the overlay ends at lyrics.last().frame instead of the preview endFrame, which can extend preview playback length beyond the intended snippet.
Proposed fix
translucentMotionView(
color = "`#28000000`",
- startFrame = lyrics.first().frame,
- endFrame = lyrics.last().frame,
+ startFrame = startFrame,
+ endFrame = endFrame,
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/AccentLyricsTemplate.kt`
around lines 87 - 91, In the preview overlay inside AccentLyricsTemplate's
preview rendering, the translucentMotionView is currently using
lyrics.last().frame for endFrame which can extend playback; update the
translucentMotionView call to use the preview's frame range (use
preview.endFrame for endFrame — and if startFrame is also meant to be the
preview start, use preview.startFrame instead of lyrics.first().frame) so the
overlay matches the preview snippet.
| TimelineUtils.fromSdui(context, project.sdui) | ||
| } | ||
|
|
||
| var currentFrame by remember { mutableIntStateOf(0) } |
Contributor
There was a problem hiding this comment.
Reset currentFrame when project changes.
remember { mutableIntStateOf(0) } keeps frame from a previous project if this composable is reused with a new project.
Proposed fix
- var currentFrame by remember { mutableIntStateOf(0) }
+ var currentFrame by remember(project.id) { mutableIntStateOf(0) }📝 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.
Suggested change
| var currentFrame by remember { mutableIntStateOf(0) } | |
| var currentFrame by remember(project.id) { mutableIntStateOf(0) } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.kt`
at line 54, The currentFrame state is remembered unconditionally so it persists
across different projects; update its creation to depend on the current project
so it resets when project changes by replacing the line that declares
currentFrame (var currentFrame by remember { mutableIntStateOf(0) }) with a
remember keyed on the project (e.g., var currentFrame by remember(project) {
mutableIntStateOf(0) }) in MotionEditorScreen (so the state is recreated
whenever project changes).
Comment on lines
+166
to
+170
| private fun formatFrameToTime(frame: Int, fps: Int): String { | ||
| val totalSeconds = frame / fps | ||
| val minutes = totalSeconds / 60 | ||
| val seconds = totalSeconds % 60 | ||
| return "%02d:%02d".format(minutes, seconds) |
Contributor
There was a problem hiding this comment.
Guard against fps <= 0 to avoid divide-by-zero crash.
formatFrameToTime() divides by fps directly. If fps is zero from config/input, this will crash at runtime.
Proposed fix
private fun formatFrameToTime(frame: Int, fps: Int): String {
- val totalSeconds = frame / fps
+ val safeFps = fps.coerceAtLeast(1)
+ val totalSeconds = frame / safeFps
val minutes = totalSeconds / 60
val seconds = totalSeconds % 60
return "%02d:%02d".format(minutes, seconds)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt`
around lines 166 - 170, formatFrameToTime currently divides by fps without
validation which can cause a divide-by-zero; update the function
(formatFrameToTime) to guard against fps <= 0—if fps is <= 0, return a safe
default string like "00:00" (or alternatively throw an IllegalArgumentException)
instead of performing the division; ensure all calculations (totalSeconds,
minutes, seconds) only run when fps > 0 so the function never divides by zero.
Comment on lines
+34
to
+65
| val motionConfig = config ?: provideCurrentConfig() | ||
| setCurrentConfig(motionConfig) | ||
|
|
||
| val producer = | ||
| MotionVideoProducer.with( | ||
| context = context, | ||
| ) | ||
|
|
||
| val contentScope = | ||
| ContentScope( | ||
| context = context, | ||
| producer = producer, | ||
| data = data, | ||
| ) | ||
|
|
||
| template.buildContent(contentScope) | ||
|
|
||
| val views = mutableListOf<MotionView>() | ||
| for (i in 0 until producer.motionComposerView.childCount) { | ||
| val child = producer.motionComposerView.getChildAt(i) | ||
| if (child is MotionView) { | ||
| views.add(child) | ||
| } | ||
| } | ||
|
|
||
| return createMotionSDUIJson( | ||
| views = views, | ||
| audios = producer.motionAudio, | ||
| plugins = producer.motionComposerView.plugins, | ||
| config = motionConfig, | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
Restore the previous global config after SDUI generation.
setCurrentConfig(motionConfig) changes shared state and never resets it, so later calls can run with the wrong config.
Proposed fix
): JsonObject {
- val motionConfig = config ?: provideCurrentConfig()
- setCurrentConfig(motionConfig)
+ val previousConfig = provideCurrentConfig()
+ val motionConfig = config ?: previousConfig
+ setCurrentConfig(motionConfig)
- val producer =
- MotionVideoProducer.with(
- context = context,
- )
+ try {
+ val producer =
+ MotionVideoProducer.with(
+ context = context,
+ )
- val contentScope =
- ContentScope(
- context = context,
- producer = producer,
- data = data,
- )
+ val contentScope =
+ ContentScope(
+ context = context,
+ producer = producer,
+ data = data,
+ )
- template.buildContent(contentScope)
+ template.buildContent(contentScope)
- val views = mutableListOf<MotionView>()
- for (i in 0 until producer.motionComposerView.childCount) {
- val child = producer.motionComposerView.getChildAt(i)
- if (child is MotionView) {
- views.add(child)
+ val views = mutableListOf<MotionView>()
+ for (i in 0 until producer.motionComposerView.childCount) {
+ val child = producer.motionComposerView.getChildAt(i)
+ if (child is MotionView) {
+ views.add(child)
+ }
}
- }
- return createMotionSDUIJson(
- views = views,
- audios = producer.motionAudio,
- plugins = producer.motionComposerView.plugins,
- config = motionConfig,
- )
+ return createMotionSDUIJson(
+ views = views,
+ audios = producer.motionAudio,
+ plugins = producer.motionComposerView.plugins,
+ config = motionConfig,
+ )
+ } finally {
+ setCurrentConfig(previousConfig)
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/sdui/MotionTemplateSDUIProvider.kt`
around lines 34 - 65, The code sets a global config via
setCurrentConfig(motionConfig) and never resets it, leading to leaked shared
state; to fix, capture the previous config (e.g., val previous =
provideCurrentConfig()) before calling setCurrentConfig(motionConfig), run the
SDUI generation (MotionVideoProducer.with(...), template.buildContent(...),
createMotionSDUIJson(...)) inside a try block, and restore the previous config
in a finally block via setCurrentConfig(previous) so the global config is always
restored even on errors.
012e3d2 to
15ecdc3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt (3)
94-96:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftRemove
fillMaxWidth()from tracks to enable horizontal scrolling beyond viewport.At line 180,
TimelineTrackViewusesfillMaxWidth(), which constrains each track to the viewport width. Items are positioned withoffset()(line 201), which doesn't expand the measured layout width—it only repositions after measurement. This leaves zero scrollable range beyond the viewport, making off-screen items unreachable.Compute the maximum timeline extent from item data and apply an explicit width to each track:
🔧 Proposed fix
`@Composable` fun MotionTimeline( tracks: List<TimelineTrack>, currentFrame: Int, totalFrames: Int, onFrameChange: (Int) -> Unit, fps: Int = 30, pixelsPerFrame: Float = 5f, modifier: Modifier = Modifier ) { val horizontalScrollState = rememberScrollState() val verticalScrollState = rememberScrollState() val density = LocalDensity.current + + // Compute timeline width from content + val maxEndFrame = tracks.flatMap { it.items }.maxOfOrNull { it.endFrame } ?: totalFrames + val timelineWidthDp = (maxEndFrame * pixelsPerFrame).dp // ... existing LaunchedEffect ... Column( modifier = Modifier .fillMaxSize() .horizontalScroll(horizontalScrollState) .verticalScroll(verticalScrollState) .padding(vertical = 16.dp) ) { tracks.forEach { track -> - TimelineTrackView(track, pixelsPerFrame) + TimelineTrackView( + track = track, + pixelsPerFrame = pixelsPerFrame, + modifier = Modifier.width(timelineWidthDp) + ) } } @@ `@Composable` fun TimelineTrackView( track: TimelineTrack, - pixelsPerFrame: Float + pixelsPerFrame: Float, + modifier: Modifier = Modifier ) { Box( - modifier = Modifier - .fillMaxWidth() + modifier = modifier .height(50.dp)Also applies to: 174-189
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt` around lines 94 - 96, The tracks currently use fillMaxWidth() inside TimelineTrackView which forces each track to viewport width and prevents horizontal scrolling—remove fillMaxWidth() from TimelineTrackView and instead compute the timeline's max extent (e.g., iterate items to find max end frame) and set an explicit width on the track container using that extent multiplied by pixelsPerFrame; keep using offset() for item positioning but ensure the container width comes from the computed maxExtent so off-screen items become scrollable.
166-171:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard against
fps <= 0to avoid divide-by-zero crash.
formatFrameToTime()divides byfpsat line 167. Iffpsis zero or negative from caller input, this will crash at runtime.🛡️ Proposed fix
private fun formatFrameToTime(frame: Int, fps: Int): String { + val safeFps = fps.coerceAtLeast(1) - val totalSeconds = frame / fps + val totalSeconds = frame / safeFps val minutes = totalSeconds / 60 val seconds = totalSeconds % 60 return "%02d:%02d".format(minutes, seconds) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt` around lines 166 - 171, The function formatFrameToTime currently divides by fps and can crash when fps <= 0; add an input guard at the start of formatFrameToTime to validate fps and handle invalid values (e.g., if fps <= 0) by throwing an IllegalArgumentException with a clear message that includes the invalid fps value, or alternatively clamp fps to a safe minimum (1) before computing totalSeconds; update callers if you choose to throw so invalid fps is surfaced early.
192-215:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify density conversion and guard against negative item duration.
Lines 196–197 apply
.dpdirectly to(item.startFrame * pixelsPerFrame)and(item.endFrame - item.startFrame) * pixelsPerFrame. The parameter is namedpixelsPerFrame(suggesting screen pixels), but.dptreats the value as density-independent units without conversion. Additionally, ifendFrame < startFrame, the width becomes negative, causing layout issues.If
pixelsPerFramerepresents screen pixels, use density-aware conversion. If it represents dp (despite the name), the current approach is correct but the parameter should be renamed for clarity.Run the following script to check how
pixelsPerFrameis documented and used across the codebase:#!/bin/bash # Search for pixelsPerFrame parameter declarations and usage patterns rg -n "pixelsPerFrame" -B2 -A2 --type kotlin🔧 Proposed fix (if pixelsPerFrame is in screen pixels)
+import androidx.compose.ui.platform.LocalDensity @@ `@Composable` fun TimelineItemView( item: TimelineItem, pixelsPerFrame: Float ) { + val density = LocalDensity.current + val durationFrames = (item.endFrame - item.startFrame).coerceAtLeast(0) - val startPx = (item.startFrame * pixelsPerFrame).dp - val widthPx = ((item.endFrame - item.startFrame) * pixelsPerFrame).dp + val startDp = with(density) { (item.startFrame * pixelsPerFrame).toDp() } + val widthDp = with(density) { (durationFrames * pixelsPerFrame).toDp() } Box( modifier = Modifier - .offset(x = startPx) - .width(widthPx) + .offset(x = startDp) + .width(widthDp)♻️ Alternative: rename parameter if it's actually in dp units
If
pixelsPerFrameis meant to represent dp units, rename it todpPerFrameorzoomFactorand add a KDoc to clarify. The current.dpconversion would then be correct.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt` around lines 192 - 215, The TimelineItemView currently multiplies frames by pixelsPerFrame and directly calls .dp which is wrong if pixelsPerFrame is in screen pixels and also allows negative widths; update TimelineItemView to convert pixels to dp using the composition density (LocalDensity.current) when pixelsPerFrame represents screen pixels, compute startDp and widthDp via density.run { ...toDp() }, and clamp the computed width (from (item.endFrame - item.startFrame) * pixelsPerFrame) to non-negative (max with 0) before converting; alternatively, if pixelsPerFrame is actually in dp units, rename the parameter to dpPerFrame or zoomFactor and add KDoc to clarify so the direct .dp usage is correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt`:
- Around line 48-54: The auto-scroll target computed in the LaunchedEffect for
currentFrame can be negative when currentFrame is near zero; update the
calculation before calling horizontalScrollState.animateScrollTo to clamp the
target to >= 0 (e.g., use Math.max or Kotlin's .coerceAtLeast on the value
computed from currentFrame * pixelsPerFrame * density.density minus
viewportWidth / 2) so the animateScrollTo invocation always receives a
non-negative scroll offset; keep this change inside the existing LaunchedEffect
that references currentFrame, pixelsPerFrame, density, and
horizontalScrollState.
---
Duplicate comments:
In
`@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt`:
- Around line 94-96: The tracks currently use fillMaxWidth() inside
TimelineTrackView which forces each track to viewport width and prevents
horizontal scrolling—remove fillMaxWidth() from TimelineTrackView and instead
compute the timeline's max extent (e.g., iterate items to find max end frame)
and set an explicit width on the track container using that extent multiplied by
pixelsPerFrame; keep using offset() for item positioning but ensure the
container width comes from the computed maxExtent so off-screen items become
scrollable.
- Around line 166-171: The function formatFrameToTime currently divides by fps
and can crash when fps <= 0; add an input guard at the start of
formatFrameToTime to validate fps and handle invalid values (e.g., if fps <= 0)
by throwing an IllegalArgumentException with a clear message that includes the
invalid fps value, or alternatively clamp fps to a safe minimum (1) before
computing totalSeconds; update callers if you choose to throw so invalid fps is
surfaced early.
- Around line 192-215: The TimelineItemView currently multiplies frames by
pixelsPerFrame and directly calls .dp which is wrong if pixelsPerFrame is in
screen pixels and also allows negative widths; update TimelineItemView to
convert pixels to dp using the composition density (LocalDensity.current) when
pixelsPerFrame represents screen pixels, compute startDp and widthDp via
density.run { ...toDp() }, and clamp the computed width (from (item.endFrame -
item.startFrame) * pixelsPerFrame) to non-negative (max with 0) before
converting; alternatively, if pixelsPerFrame is actually in dp units, rename the
parameter to dpPerFrame or zoomFactor and add KDoc to clarify so the direct .dp
usage is correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86b069b4-a43b-4939-ab80-4fcc4cac9b60
⛔ Files ignored due to path filters (1)
modules/lyrics-maker/src/main/assets/fonts/Malam_Poek.ttfis excluded by!**/*.ttf
📒 Files selected for processing (29)
.idea/gradle.xmlgradle/libs.versions.tomlmodules/lyrics-maker/build.gradlemodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/AppNavHost.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/GradientTextCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/LyricsTemplateSelector.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectsScreenCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/AccentLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/RainbowLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/ZoomLyricsTemplate.ktmodules/motion-video-editor/build.gradlemodules/motion-video-editor/consumer-rules.promodules/motion-video-editor/proguard-rules.promodules/motion-video-editor/src/main/AndroidManifest.xmlmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/TimelineData.ktmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.ktmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.ktmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/utils/TimelineUtils.ktmodules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/background/TranslucentMotionView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayer.ktmodules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionSduiInitializer.ktmodules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/MotionSduiRegistryTest.ktmodules/templates/src/androidTest/java/com/tejpratapsingh/motionlib/templates/sdui/MotionTemplateSDUIProviderTest.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/BackgroundExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/sdui/MotionTemplateSDUIProvider.ktsettings.gradle
✅ Files skipped from review due to trivial changes (3)
- settings.gradle
- modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/TimelineData.kt
- modules/motion-video-editor/src/main/AndroidManifest.xml
🚧 Files skipped from review as they are similar to previous changes (21)
- .idea/gradle.xml
- modules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/MotionSduiRegistryTest.kt
- modules/lyrics-maker/build.gradle
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectsScreenCompose.kt
- modules/templates/src/androidTest/java/com/tejpratapsingh/motionlib/templates/sdui/MotionTemplateSDUIProviderTest.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/AccentLyricsTemplate.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayer.kt
- modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/utils/TimelineUtils.kt
- gradle/libs.versions.toml
- modules/motion-video-editor/build.gradle
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/BackgroundExtensions.kt
- modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/sdui/MotionTemplateSDUIProvider.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/RainbowLyricsTemplate.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsCompose.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/GradientTextCompose.kt
- modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionSduiInitializer.kt
- modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/AppNavHost.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/LyricsTemplateSelector.kt
tejpratap46
force-pushed
the
feat/motion-video-editor
branch
from
15ecdc3 to
a7b700d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt (2)
87-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack width should be explicit timeline width, not viewport width.
Using
fillMaxWidth()inTimelineTrackViewcouples track width to viewport, while items are positioned by frame offsets. This can make long-track scrolling and marker alignment inconsistent versus the ruler width.Proposed fix
fun MotionTimeline( @@ ) { val horizontalScrollState = rememberScrollState() val verticalScrollState = rememberScrollState() val density = LocalDensity.current + val timelineWidth = (totalFrames * pixelsPerFrame).dp @@ Column( modifier = Modifier - .fillMaxSize() + .width(timelineWidth) .horizontalScroll(horizontalScrollState) .verticalScroll(verticalScrollState) .padding(vertical = 16.dp) ) { tracks.forEach { track -> - TimelineTrackView(track, pixelsPerFrame) + TimelineTrackView(track, pixelsPerFrame, modifier = Modifier.width(timelineWidth)) } } @@ fun TimelineTrackView( track: TimelineTrack, - pixelsPerFrame: Float + pixelsPerFrame: Float, + modifier: Modifier = Modifier ) { Box( - modifier = Modifier - .fillMaxWidth() + modifier = modifier .height(50.dp) .padding(vertical = 4.dp) .background(Color.DarkGray.copy(alpha = 0.1f)) ) {Also applies to: 174-181
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt` around lines 87 - 96, The tracks currently use fillMaxWidth() via TimelineTrackView which ties each track to the viewport width and breaks frame-based positioning; compute and pass an explicit timeline width (e.g. timelineWidth = pixelsPerFrame * totalFrames or a provided rulerWidth) into TimelineTrackView and use Modifier.width(timelineWidth) inside the TimelineTrackView implementation (instead of fillMaxWidth()); update both call sites where TimelineTrackView is used (the tracks.forEach block and the other occurrence around lines 174-181) to accept the new timelineWidth parameter so markers, items and ruler remain aligned during horizontal scrolling.
153-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
fpsbefore using it instepand division.
fps <= 0will crash atstep fpsand atframe / fps. Validate once and short-circuit with a safe fallback.Proposed fix
`@Composable` fun TimeScaleView( totalFrames: Int, fps: Int, pixelsPerFrame: Float, modifier: Modifier = Modifier ) { + if (fps <= 0) { + Box( + modifier = modifier + .width((totalFrames * pixelsPerFrame).dp) + .height(40.dp) + ) + return + } + val density = LocalDensity.current val totalWidth = (totalFrames * pixelsPerFrame).dp @@ private fun formatFrameToTime(frame: Int, fps: Int): String { + if (fps <= 0) return "00:00" val totalSeconds = frame / fps val minutes = totalSeconds / 60 val seconds = totalSeconds % 60 return "%02d:%02d".format(minutes, seconds) }Also applies to: 166-170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt` at line 153, Guard against non-positive fps before it's used in the loops and divisions: validate the fps variable at the start of the relevant method in MotionTimeline (the code that iterates "for (frame in 0..totalFrames step fps)" and the block that uses "frame / fps" around lines 153 and 166-170). If fps <= 0, either short-circuit (return or skip rendering) or assign a safe fallback like 1 to a local safeFps and use that (replace uses of fps in step and any frame / fps calculations with safeFps) so the step operator and integer division never receive zero or negative values. Ensure the validation occurs once and that all occurrences in that method reference the validated safeFps.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SearchLyricsCompose.kt`:
- Around line 111-173: The Card currently renders all results eagerly by using
state.lyrics.forEachIndexed inside a single LazyColumn item; replace that block
with a LazyColumn itemsIndexed(state.lyrics) usage so each row is composed
lazily. Locate the block that builds ListItem entries (the forEachIndexed loop
that creates ListItem, HorizontalDivider, and calls onLyricsSelected) and move
that per-lyrics rendering into an itemsIndexed lambda (keeping the same
ListItem, Divider, modifiers, colors, and onLyricsSelected call) so each lyric
is virtualized instead of all being composed at once.
---
Duplicate comments:
In
`@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt`:
- Around line 87-96: The tracks currently use fillMaxWidth() via
TimelineTrackView which ties each track to the viewport width and breaks
frame-based positioning; compute and pass an explicit timeline width (e.g.
timelineWidth = pixelsPerFrame * totalFrames or a provided rulerWidth) into
TimelineTrackView and use Modifier.width(timelineWidth) inside the
TimelineTrackView implementation (instead of fillMaxWidth()); update both call
sites where TimelineTrackView is used (the tracks.forEach block and the other
occurrence around lines 174-181) to accept the new timelineWidth parameter so
markers, items and ruler remain aligned during horizontal scrolling.
- Line 153: Guard against non-positive fps before it's used in the loops and
divisions: validate the fps variable at the start of the relevant method in
MotionTimeline (the code that iterates "for (frame in 0..totalFrames step fps)"
and the block that uses "frame / fps" around lines 153 and 166-170). If fps <=
0, either short-circuit (return or skip rendering) or assign a safe fallback
like 1 to a local safeFps and use that (replace uses of fps in step and any
frame / fps calculations with safeFps) so the step operator and integer division
never receive zero or negative values. Ensure the validation occurs once and
that all occurrences in that method reference the validated safeFps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df92ddba-17bd-4af2-b7d2-b6a2fabe7d83
⛔ Files ignored due to path filters (1)
modules/lyrics-maker/src/main/assets/fonts/Malam_Poek.ttfis excluded by!**/*.ttf
📒 Files selected for processing (32)
.idea/gradle.xmlgradle/libs.versions.tomlmodules/lyrics-maker/build.gradlemodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/AppNavHost.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/GradientTextCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/LyricsTemplateSelector.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectsScreenCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SearchLyricsCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/AccentLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/RainbowLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/ZoomLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.ktmodules/motion-video-editor/build.gradlemodules/motion-video-editor/consumer-rules.promodules/motion-video-editor/proguard-rules.promodules/motion-video-editor/src/main/AndroidManifest.xmlmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/TimelineData.ktmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.ktmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.ktmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/utils/TimelineUtils.ktmodules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/background/TranslucentMotionView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayer.ktmodules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionSduiInitializer.ktmodules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/MotionSduiRegistryTest.ktmodules/templates/src/androidTest/java/com/tejpratapsingh/motionlib/templates/sdui/MotionTemplateSDUIProviderTest.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/BackgroundExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/sdui/MotionTemplateSDUIProvider.ktsettings.gradle
✅ Files skipped from review due to trivial changes (2)
- .idea/gradle.xml
- modules/motion-video-editor/src/main/AndroidManifest.xml
🚧 Files skipped from review as they are similar to previous changes (22)
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.kt
- modules/lyrics-maker/build.gradle
- gradle/libs.versions.toml
- modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/TimelineData.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsCompose.kt
- modules/motion-video-editor/build.gradle
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectsScreenCompose.kt
- modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/sdui/MotionTemplateSDUIProvider.kt
- settings.gradle
- modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/utils/TimelineUtils.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/GradientTextCompose.kt
- modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionSduiInitializer.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/ZoomLyricsTemplate.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/BackgroundExtensions.kt
- modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/LyricsTemplateSelector.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayer.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/AccentLyricsTemplate.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/RainbowLyricsTemplate.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/background/TranslucentMotionView.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/AppNavHost.kt
tejpratap46
force-pushed
the
feat/motion-video-editor
branch
3 times, most recently
from
cc50cc2 to
525aa59
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/MultiLyricsContainer.kt (1)
52-83:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid blocking UI initialization with
runBlocking.Line 52 performs synchronous blocking work (including network calls) in a view init path; this can freeze rendering and risks ANRs.
Proposed fix (move fetch off the main thread and cancel with view lifecycle)
-import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel +import kotlinx.coroutines.launch +import kotlinx.coroutines.withContext class MultiLyricsContainer( @@ ) : BaseFrameMotionView(context) { + private val viewScope = CoroutineScope(SupervisorJob() + Dispatchers.Main.immediate) @@ - ivAlbumArt.apply { - runBlocking { - var bitmap: Bitmap? = null - if (image != null) { - bitmap = repository.getAlbumArtBitmap(image) - } else { - Timber.i("Fetching from musicbrainz") - val songDetails = songName.split(" - ") - if (songDetails.size >= 2) { - val url = - repository.getAlbumArtUrl( - songDetails[0], - songDetails[1], - ) - - url?.let { url -> - bitmap = repository.getAlbumArtBitmap(url) - } - } - } - - if (bitmap == null) { - bitmap = - BitmapFactory.decodeResource( - context.resources, - com.tejpratapsingh.motionlib.R.drawable.default_bg, - ) - } - - bitmap?.let { - setImageBitmap(it) - } - } - } + ivAlbumArt.apply { + viewScope.launch { + val bitmap = + withContext(Dispatchers.IO) { + var resolved: Bitmap? = null + if (image != null) { + resolved = repository.getAlbumArtBitmap(image) + } else { + Timber.i("Fetching from musicbrainz") + val songDetails = songName.split(" - ") + if (songDetails.size >= 2) { + val url = repository.getAlbumArtUrl(songDetails[0], songDetails[1]) + url?.let { resolved = repository.getAlbumArtBitmap(it) } + } + } + resolved ?: BitmapFactory.decodeResource( + context.resources, + com.tejpratapsingh.motionlib.R.drawable.default_bg, + ) + } + setImageBitmap(bitmap) + } + } } + + override fun onDetachedFromWindow() { + viewScope.cancel() + super.onDetachedFromWindow() + }#!/bin/bash # Verify blocking coroutine usage in this view path. rg -nP --type=kt -C2 '\brunBlocking\s*\{' modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/MultiLyricsContainer.kt🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/MultiLyricsContainer.kt` around lines 52 - 83, The code currently blocks UI with runBlocking in MultiLyricsContainer; change this to perform image/network work off the main thread using a CoroutineScope tied to the view lifecycle (for example create a private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Main) in the view and cancel it in onDetachedFromWindow()), then launch a coroutine that switches to Dispatchers.IO to call repository.getAlbumArtBitmap and repository.getAlbumArtUrl, return to Dispatchers.Main to decode default BitmapFactory.decodeResource and call setImageBitmap; ensure all repository/network calls happen on IO and that the scope is cancelled when the view is detached so the work is cancellable and won’t block initialization.
♻️ Duplicate comments (10)
modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt (3)
171-171:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard
fpsbefore using it instepand time conversion.
fps <= 0will crash atstep fpsand informatFrameToTime(frame / fps).Proposed fix
`@Composable` fun TimeScaleView( @@ ) { + val safeFps = fps.coerceAtLeast(1) @@ - for (frame in 0..totalFrames step fps) { + for (frame in 0..totalFrames step safeFps) { val x = (frame * pixelsPerFrame).dp Text( - text = formatFrameToTime(frame, fps), + text = formatFrameToTime(frame, safeFps), @@ private fun formatFrameToTime(frame: Int, fps: Int): String { + if (fps <= 0) return "00:00" val totalSeconds = frame / fpsAlso applies to: 184-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt` at line 171, The loop and time conversion use fps without validating it (see the for loop `for (frame in 0..totalFrames step fps)` and calls like `formatFrameToTime(frame / fps)`), so add a guard that ensures `fps` > 0 before using it: validate `fps` at the start of the relevant function or before these loops, and if <= 0 either return early, set a safe default (e.g., 1) or skip the loop; apply the same guard to the other occurrences referenced around lines 184–188 to prevent crashes and divide-by-zero errors.
196-201:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack width is not tied to timeline duration, so horizontal scrolling can under-measure content.
TimelineTrackViewstill usesfillMaxWidth(), while item placement usesoffset; that can leave scroll range inconsistent with actual timeline content width.Proposed fix
`@Composable` fun MotionTimeline( @@ ) { + val timelineWidth = (totalFrames * pixelsPerFrame).dp @@ tracks.forEach { track -> Row { Spacer(modifier = Modifier.width(halfWidth)) - TimelineTrackView(track, pixelsPerFrame) + TimelineTrackView( + track = track, + pixelsPerFrame = pixelsPerFrame, + modifier = Modifier.width(timelineWidth) + ) Spacer(modifier = Modifier.width(halfWidth)) } } @@ fun TimelineTrackView( track: TimelineTrack, - pixelsPerFrame: Float + pixelsPerFrame: Float, + modifier: Modifier = Modifier ) { Box( - modifier = Modifier - .fillMaxWidth() + modifier = modifier .height(50.dp)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt` around lines 196 - 201, The Box in TimelineTrackView uses fillMaxWidth() which disconnects the track container width from the actual timeline content placed with offsets; compute an explicit timeline content width (e.g., timelineWidth = durationSeconds * pixelsPerSecond or a similar pixelsPerUnit value) and replace fillMaxWidth() with a fixed Modifier.width(timelineWidth) (or wrapContent with minWidth = timelineWidth) so horizontal scrolling and the layout's scroll range match the offset-based item placement; update any parent scrollable container to use that same timelineWidth when measuring/scrolling.
214-215:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent negative widths for invalid frame ranges.
When
item.endFrame < item.startFrame, width becomes negative and produces invalid layout behavior.Proposed fix
- val widthPx = ((item.endFrame - item.startFrame) * pixelsPerFrame).dp + val durationFrames = (item.endFrame - item.startFrame).coerceAtLeast(0) + val widthPx = (durationFrames * pixelsPerFrame).dp🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt` around lines 214 - 215, The width calculation can become negative when item.endFrame < item.startFrame; update the calculations in MotionTimeline (where startPx and widthPx are computed) to clamp the frame range: compute startFrameClamped = min(item.startFrame, item.endFrame) and widthFrames = max(0, item.endFrame - item.startFrame) (or use max(0, end - start) directly), then compute startPx = (startFrameClamped * pixelsPerFrame).dp and widthPx = (widthFrames * pixelsPerFrame).dp so widthPx is never negative and the layout remains valid.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/AccentLyricsTemplate.kt (1)
88-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse preview frame bounds for preview overlay.
In preview mode, the overlay still uses full lyrics bounds; it should use the preview
startFrame/endFrameto match the shown snippet.Proposed fix
translucentMotionView( color = "`#000000`", alpha = 0.4f, - startFrame = lyrics.first().frame, - endFrame = lyrics.last().frame, + startFrame = startFrame, + endFrame = endFrame, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/AccentLyricsTemplate.kt` around lines 88 - 93, The translucent overlay currently uses the full lyrics bounds by passing lyrics.first().frame and lyrics.last().frame to translucentMotionView; when rendering a preview it should use the preview snippet bounds instead. Update the call site of translucentMotionView to pass the preview start/end frames (the preview's startFrame and endFrame variables) when in preview mode (instead of lyrics.first().frame and lyrics.last().frame), ensuring translucentMotionView receives the preview startFrame and endFrame so the overlay matches the shown snippet.modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt (3)
80-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPlayback can drift from externally controlled frame updates.
The loop initializes
currentFrameTrackeronce fromeffectiveCurrentFrame, so parent-driven seeks during playback can be overwritten by stale progression.Also applies to: 94-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt` around lines 80 - 86, The playback loop in the LaunchedEffect for effectiveIsPlaying uses a single-initialized currentFrameTracker (from effectiveCurrentFrame) which allows parent-driven seeks to be overwritten; change the loop so it reads effectiveCurrentFrame on each iteration (or when detecting a change) instead of only once, and update lastFrameTime/currentFrameTracker accordingly to respect external seeks; specifically modify the while loop inside LaunchedEffect(effectiveIsPlaying) to reference effectiveCurrentFrame each tick (and adjust the logic around lastFrameTime and frameDurationMs) so the progression never clobbers externally controlled frame updates (also apply identical fix to the similar logic around lines 94-99).
224-299:⚠️ Potential issue | 🟠 Major | ⚡ Quick winControls and timer can overlap on narrow screens / large font scales.
The centered control row and
Alignment.CenterEndtimer share the sameBox, so they can collide in constrained layouts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt` around lines 224 - 299, The controls Row and the timer Text are in the same Box and can overlap in narrow layouts; change the layout so the controls and timer are placed in a single Row with spacing/weights or separate containers to avoid overlap: replace the outer Box containing Row and Text with a Row (or keep Box but put a full-width Row) where the controls Row is in a start-aligned container and the timer Text is end-aligned using Spacer(Modifier.weight(1f)) or Modifier.weight on children, ensuring IconButton group, effectiveCurrentFrame/totalFrames Text and formatting via formatTime(motionConfig.fps) stay intact and retain the existing click handlers (onFrameChange, onPlayingChange) and effectiveIsPlaying logic.
130-135:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
MediaPlayersetup calls to avoid runtime crashes.
setDataSource()andprepare()can throw at runtime (missing/corrupt file, invalid source) and currently crash this coroutine path.#!/bin/bash # Inspect unguarded MediaPlayer setup calls and nearby context rg -n -C3 'MediaPlayer\(\)|setDataSource\(|prepare\(' modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt rg -n 'try\s*\{' modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt` around lines 130 - 135, The MediaPlayer construction and setup (MediaPlayer, setDataSource(audio.file.absolutePath), prepare(), start()) must be guarded to avoid uncaught runtime exceptions: wrap the setup in a try-catch around setDataSource and prepare, call start() only if prepare succeeded, and ensure mediaPlayer.release() is called in a finally block (or on failure) to avoid resource leaks; catch and log the exception (or emit an error state) instead of letting it crash the coroutine path in MotionVideoPlayerCompose.kt.modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.kt (2)
47-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap producer/timeline construction to avoid hard crashes on malformed project SDUI.
Both
createFromProject(project)andTimelineUtils.fromSdui(...)are called inrememberwithout failure handling; invalid SDUI can crash this screen.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.kt` around lines 47 - 54, Wrap the calls that can throw when building motionVideoProducer and timelineTracks (specifically producerFactory.createFromProject(project) and TimelineUtils.fromSdui(context, project.sdui)) in try/catch inside their remember blocks; on exception log the error and return a safe fallback (e.g., a no-op/empty producer instance for motionVideoProducer and an empty Timeline/track list for timelineTracks) so the UI doesn't crash, and ensure the remember keys remain the same (keep project as the key) so failures are handled per project change.
56-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset frame state when the project changes.
remember { mutableIntStateOf(0) }can retain a prior project’s frame if this composable is reused.Proposed patch
- var currentFrame by remember { mutableIntStateOf(0) } + var currentFrame by remember(project.id) { mutableIntStateOf(0) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.kt` at line 56, The currentFrame state defined as "var currentFrame by remember { mutableIntStateOf(0) }" can persist across projects; update it to reset when the project changes by wiring the project (or a stable project identifier) into the remember key—e.g. use remember(project.id) or remember(project) { mutableIntStateOf(0) } inside the MotionEditorScreen composable so currentFrame is reinitialized whenever the project changes.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.kt (1)
23-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden lyrics metadata parsing before unsafe Gson casts.
The mapping still assumes each array item is a JSON object and directly casts fields; malformed entries can throw and break producer creation.
Proposed patch
- val lyrics = - motionProject.metadata.get("lyrics")?.takeIf { it.isJsonArray }?.asJsonArray?.map { - SyncedLyricFrame( - frame = - it.asJsonObject - .get("frame") - ?.takeIf { f -> f.isJsonPrimitive } - ?.asInt ?: 0, - text = - it.asJsonObject - .get("text") - ?.takeIf { t -> t.isJsonPrimitive } - ?.asString ?: "", - ) - } ?: emptyList() + val lyrics = + motionProject.metadata.get("lyrics") + ?.takeIf { it.isJsonArray } + ?.asJsonArray + ?.mapNotNull { element -> + val obj = element.takeIf { it.isJsonObject }?.asJsonObject ?: return@mapNotNull null + SyncedLyricFrame( + frame = + obj.get("frame") + ?.takeIf { f -> f.isJsonPrimitive && f.asJsonPrimitive.isNumber } + ?.asInt ?: 0, + text = + obj.get("text") + ?.takeIf { t -> t.isJsonPrimitive } + ?.asString ?: "", + ) + } ?: emptyList()#!/bin/bash # Verify current unsafe cast points in this file rg -n -C2 'asJsonObject|asInt|asString' modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.kt🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.kt` around lines 23 - 35, The current mapping of motionProject.metadata to SyncedLyricFrame assumes each array element is a JSON object and directly uses asJsonObject/asInt/asString which can throw on malformed entries; update the mapping in MultiLyricsVideoProducer to first filter/guard each element with isJsonObject, safely retrieve "frame" and "text" using null checks and type checks (e.g., get("frame")?.takeIf { it.isJsonPrimitive }?.asInt ?: 0 and get("text")?.takeIf { it.isJsonPrimitive }?.asString ?: "") or skip the element entirely when invalid, so SyncedLyricFrame construction only happens for valid JSON objects and malformed entries are ignored or defaulted.
🧹 Nitpick comments (3)
modules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/MotionSduiRegistryTest.kt (1)
76-76: ⚡ Quick winUse tolerant float assertion for alpha.
Direct float
==is brittle; useassertEquals(expected, actual, delta).Proposed fix
+import org.junit.Assert.assertEquals @@ - assertTrue((view as TranslucentMotionView).alpha == 0.5f) + assertEquals(0.5f, (view as TranslucentMotionView).alpha, 0.0001f)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/MotionSduiRegistryTest.kt` at line 76, The test uses a brittle exact float comparison in the assertion for TranslucentMotionView.alpha; replace the assertTrue((view as TranslucentMotionView).alpha == 0.5f) with a tolerant float assertion using assertEquals(expected, actual, delta) (e.g., assertEquals(0.5f, (view as TranslucentMotionView).alpha, 1e-6f)) so small floating-point imprecision won't break the test.modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/utils/TimelineUtils.kt (1)
24-24: ⚡ Quick winAvoid using
hashCode()as timeline item IDs.
hashCode()is not stable across model reconstruction, so IDs can drift between renders and break stateful timeline interactions. Use a deterministic ID from stable fields.Proposed fix
- id = view.hashCode().toString(), + id = "${view.javaClass.simpleName}_${index}_${view.startFrame}_${view.endFrame}",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/utils/TimelineUtils.kt` at line 24, The timeline item ID is being set to view.hashCode().toString() in TimelineUtils (unstable across re-renders); replace this with a deterministic stable identifier derived from an immutable field on the model/view (e.g., a persistent model ID, view.id, view.tag that you set once, or a stored UUID) so IDs remain stable between reconstructs; update the code that builds timeline items (where id = view.hashCode().toString()) to use that stable field (or a persisted mapping from view to generated UUID) and ensure any consumers expect the new stable ID format.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/WordwriterLyricsTemplate.kt (1)
30-35: ⚡ Quick winGuard image rendering for blank values, not just null.
image?.let { ... }still executes for"", which can pass an empty URI intomotionImageView. PreferisNotBlank()gating in both content and preview blocks.Proposed patch
- image?.let { + image?.takeIf { it.isNotBlank() }?.let { imagePath -> motionImageView( startFrame = lyrics.first().frame, endFrame = lyrics.last().frame, - imageUri = image.toUri(), + imageUri = imagePath.toUri(), ) } @@ - image?.let { + image?.takeIf { it.isNotBlank() }?.let { imagePath -> motionImageView( startFrame = previewLyrics.first().frame, endFrame = previewLyrics.last().frame, - imageUri = image.toUri(), + imageUri = imagePath.toUri(), ) }Also applies to: 73-78
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/WordwriterLyricsTemplate.kt` around lines 30 - 35, The current image guard uses null-check image?.let { ... } which still runs for empty or whitespace strings and can pass an empty URI into motionImageView; update both the content and preview blocks to check image.isNullOrBlank() (or image?.isNotBlank() == true) before calling motionImageView (references: the image?.let { ... } blocks and the motionImageView(startFrame = lyrics.first().frame, endFrame = lyrics.last().frame, imageUri = image.toUri()) invocation) so only non-blank image strings are converted to URIs and rendered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.kt`:
- Around line 8-11: Add the missing import for Modifier.weight by importing
androidx.compose.foundation.layout.weight; locate the usage of
Modifier.weight(1f) in MotionEditorScreen (the composable function
MotionEditorScreen) and add the androidx.compose.foundation.layout.weight import
alongside the other layout imports so the file compiles.
In
`@modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt`:
- Around line 14-15: The preview's Box uses Modifier.weight(1f) but the import
for weight is missing; add the missing import for
androidx.compose.foundation.layout.weight so Modifier.weight(1f) resolves (look
for the preview Box/Modifier.weight usage around the preview function in
MotionVideoPlayerCompose.kt and add the import alongside the other
androidx.compose.foundation.layout imports).
In
`@modules/templates/src/androidTest/java/com/tejpratapsingh/motionlib/templates/sdui/MotionTemplateSDUIProviderTest.kt`:
- Line 34: The test is passing File(context.cacheDir, "test.mp3") to audio(...)
without creating it first; update MotionTemplateSDUIProviderTest so before
calling audio(...) you explicitly create the test file (e.g., call
createNewFile() or writeBytes(...) on File(context.cacheDir, "test.mp3")) and
verify it exists, then pass that File to audio(...); reference the File(...)
construction and the audio(...) call so the change is localized to that test and
does not alter production code.
---
Outside diff comments:
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/MultiLyricsContainer.kt`:
- Around line 52-83: The code currently blocks UI with runBlocking in
MultiLyricsContainer; change this to perform image/network work off the main
thread using a CoroutineScope tied to the view lifecycle (for example create a
private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Main) in the
view and cancel it in onDetachedFromWindow()), then launch a coroutine that
switches to Dispatchers.IO to call repository.getAlbumArtBitmap and
repository.getAlbumArtUrl, return to Dispatchers.Main to decode default
BitmapFactory.decodeResource and call setImageBitmap; ensure all
repository/network calls happen on IO and that the scope is cancelled when the
view is detached so the work is cancellable and won’t block initialization.
---
Duplicate comments:
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.kt`:
- Around line 23-35: The current mapping of motionProject.metadata to
SyncedLyricFrame assumes each array element is a JSON object and directly uses
asJsonObject/asInt/asString which can throw on malformed entries; update the
mapping in MultiLyricsVideoProducer to first filter/guard each element with
isJsonObject, safely retrieve "frame" and "text" using null checks and type
checks (e.g., get("frame")?.takeIf { it.isJsonPrimitive }?.asInt ?: 0 and
get("text")?.takeIf { it.isJsonPrimitive }?.asString ?: "") or skip the element
entirely when invalid, so SyncedLyricFrame construction only happens for valid
JSON objects and malformed entries are ignored or defaulted.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/AccentLyricsTemplate.kt`:
- Around line 88-93: The translucent overlay currently uses the full lyrics
bounds by passing lyrics.first().frame and lyrics.last().frame to
translucentMotionView; when rendering a preview it should use the preview
snippet bounds instead. Update the call site of translucentMotionView to pass
the preview start/end frames (the preview's startFrame and endFrame variables)
when in preview mode (instead of lyrics.first().frame and lyrics.last().frame),
ensuring translucentMotionView receives the preview startFrame and endFrame so
the overlay matches the shown snippet.
In
`@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.kt`:
- Around line 47-54: Wrap the calls that can throw when building
motionVideoProducer and timelineTracks (specifically
producerFactory.createFromProject(project) and TimelineUtils.fromSdui(context,
project.sdui)) in try/catch inside their remember blocks; on exception log the
error and return a safe fallback (e.g., a no-op/empty producer instance for
motionVideoProducer and an empty Timeline/track list for timelineTracks) so the
UI doesn't crash, and ensure the remember keys remain the same (keep project as
the key) so failures are handled per project change.
- Line 56: The currentFrame state defined as "var currentFrame by remember {
mutableIntStateOf(0) }" can persist across projects; update it to reset when the
project changes by wiring the project (or a stable project identifier) into the
remember key—e.g. use remember(project.id) or remember(project) {
mutableIntStateOf(0) } inside the MotionEditorScreen composable so currentFrame
is reinitialized whenever the project changes.
In
`@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.kt`:
- Line 171: The loop and time conversion use fps without validating it (see the
for loop `for (frame in 0..totalFrames step fps)` and calls like
`formatFrameToTime(frame / fps)`), so add a guard that ensures `fps` > 0 before
using it: validate `fps` at the start of the relevant function or before these
loops, and if <= 0 either return early, set a safe default (e.g., 1) or skip the
loop; apply the same guard to the other occurrences referenced around lines
184–188 to prevent crashes and divide-by-zero errors.
- Around line 196-201: The Box in TimelineTrackView uses fillMaxWidth() which
disconnects the track container width from the actual timeline content placed
with offsets; compute an explicit timeline content width (e.g., timelineWidth =
durationSeconds * pixelsPerSecond or a similar pixelsPerUnit value) and replace
fillMaxWidth() with a fixed Modifier.width(timelineWidth) (or wrapContent with
minWidth = timelineWidth) so horizontal scrolling and the layout's scroll range
match the offset-based item placement; update any parent scrollable container to
use that same timelineWidth when measuring/scrolling.
- Around line 214-215: The width calculation can become negative when
item.endFrame < item.startFrame; update the calculations in MotionTimeline
(where startPx and widthPx are computed) to clamp the frame range: compute
startFrameClamped = min(item.startFrame, item.endFrame) and widthFrames = max(0,
item.endFrame - item.startFrame) (or use max(0, end - start) directly), then
compute startPx = (startFrameClamped * pixelsPerFrame).dp and widthPx =
(widthFrames * pixelsPerFrame).dp so widthPx is never negative and the layout
remains valid.
In
`@modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt`:
- Around line 80-86: The playback loop in the LaunchedEffect for
effectiveIsPlaying uses a single-initialized currentFrameTracker (from
effectiveCurrentFrame) which allows parent-driven seeks to be overwritten;
change the loop so it reads effectiveCurrentFrame on each iteration (or when
detecting a change) instead of only once, and update
lastFrameTime/currentFrameTracker accordingly to respect external seeks;
specifically modify the while loop inside LaunchedEffect(effectiveIsPlaying) to
reference effectiveCurrentFrame each tick (and adjust the logic around
lastFrameTime and frameDurationMs) so the progression never clobbers externally
controlled frame updates (also apply identical fix to the similar logic around
lines 94-99).
- Around line 224-299: The controls Row and the timer Text are in the same Box
and can overlap in narrow layouts; change the layout so the controls and timer
are placed in a single Row with spacing/weights or separate containers to avoid
overlap: replace the outer Box containing Row and Text with a Row (or keep Box
but put a full-width Row) where the controls Row is in a start-aligned container
and the timer Text is end-aligned using Spacer(Modifier.weight(1f)) or
Modifier.weight on children, ensuring IconButton group,
effectiveCurrentFrame/totalFrames Text and formatting via
formatTime(motionConfig.fps) stay intact and retain the existing click handlers
(onFrameChange, onPlayingChange) and effectiveIsPlaying logic.
- Around line 130-135: The MediaPlayer construction and setup (MediaPlayer,
setDataSource(audio.file.absolutePath), prepare(), start()) must be guarded to
avoid uncaught runtime exceptions: wrap the setup in a try-catch around
setDataSource and prepare, call start() only if prepare succeeded, and ensure
mediaPlayer.release() is called in a finally block (or on failure) to avoid
resource leaks; catch and log the exception (or emit an error state) instead of
letting it crash the coroutine path in MotionVideoPlayerCompose.kt.
---
Nitpick comments:
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/WordwriterLyricsTemplate.kt`:
- Around line 30-35: The current image guard uses null-check image?.let { ... }
which still runs for empty or whitespace strings and can pass an empty URI into
motionImageView; update both the content and preview blocks to check
image.isNullOrBlank() (or image?.isNotBlank() == true) before calling
motionImageView (references: the image?.let { ... } blocks and the
motionImageView(startFrame = lyrics.first().frame, endFrame =
lyrics.last().frame, imageUri = image.toUri()) invocation) so only non-blank
image strings are converted to URIs and rendered.
In
`@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/utils/TimelineUtils.kt`:
- Line 24: The timeline item ID is being set to view.hashCode().toString() in
TimelineUtils (unstable across re-renders); replace this with a deterministic
stable identifier derived from an immutable field on the model/view (e.g., a
persistent model ID, view.id, view.tag that you set once, or a stored UUID) so
IDs remain stable between reconstructs; update the code that builds timeline
items (where id = view.hashCode().toString()) to use that stable field (or a
persisted mapping from view to generated UUID) and ensure any consumers expect
the new stable ID format.
In
`@modules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/MotionSduiRegistryTest.kt`:
- Line 76: The test uses a brittle exact float comparison in the assertion for
TranslucentMotionView.alpha; replace the assertTrue((view as
TranslucentMotionView).alpha == 0.5f) with a tolerant float assertion using
assertEquals(expected, actual, delta) (e.g., assertEquals(0.5f, (view as
TranslucentMotionView).alpha, 1e-6f)) so small floating-point imprecision won't
break the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c416c0c8-d6ea-449d-8e63-878162b4644d
⛔ Files ignored due to path filters (2)
modules/lyrics-maker/src/main/assets/fonts/Malam_Poek.ttfis excluded by!**/*.ttfmodules/motionlib/src/main/res/drawable/default_bg.jpgis excluded by!**/*.jpg
📒 Files selected for processing (42)
.idea/gradle.xmlgradle/libs.versions.tomlmodules/core/src/main/java/com/tejpratapsingh/motionlib/core/extensions/KtorExtension.ktmodules/lyrics-maker/build.gradlemodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/data/api/albumart/client/AlbumArtRemoteDataSourceImpl.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/GradientTextCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/LyricsTemplateSelector.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectsScreenCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SearchLyricsCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/SyncedLyricsSelector.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/AccentLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/PopupLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/RainbowLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/TypewriterLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/VibrateLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/WordwriterLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/ZoomLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/MultiLyricsContainer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.ktmodules/motion-video-editor/build.gradlemodules/motion-video-editor/consumer-rules.promodules/motion-video-editor/proguard-rules.promodules/motion-video-editor/src/main/AndroidManifest.xmlmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/TimelineData.ktmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.ktmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionTimeline.ktmodules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/utils/TimelineUtils.ktmodules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/background/TranslucentMotionView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayer.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/utils/ImageUtil.ktmodules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionSduiInitializer.ktmodules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/MotionSduiRegistryTest.ktmodules/templates/src/androidTest/java/com/tejpratapsingh/motionlib/templates/sdui/MotionTemplateSDUIProviderTest.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/BackgroundExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/sdui/MotionTemplateSDUIProvider.ktsettings.gradle
✅ Files skipped from review due to trivial changes (2)
- .idea/gradle.xml
- modules/motion-video-editor/src/main/AndroidManifest.xml
Comment on lines
+8
to
+11
| import androidx.compose.foundation.layout.height | ||
| import androidx.compose.foundation.layout.navigationBarsPadding | ||
| import androidx.compose.foundation.layout.padding | ||
| import androidx.compose.foundation.layout.statusBarsPadding |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify usage and import presence for weight
rg -n 'Modifier\.weight\(' modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.kt
rg -n '^import androidx\.compose\.foundation\.layout\.weight$' modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 56
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.kt"
echo "=== Imports (top 60 lines) ==="
nl -ba "$FILE" | sed -n '1,60p'
echo "=== Lines around 60-90 ==="
nl -ba "$FILE" | sed -n '60,90p'
echo "=== Search: Modifier.weight( usage ==="
rg -n 'Modifier\.weight\(' "$FILE" || true
echo "=== Search: any import containing 'layout.weight' ==="
rg -n '^import .*\.layout\.weight' "$FILE" || trueRepository: tejpratap46/AndroidVideoMotion
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.kt"
echo "=== File exists? ==="
ls -l "$FILE" || exit 1
echo "=== Imports (top 80 lines) ==="
sed -n '1,80p' "$FILE"
echo "=== Lines 55-95 ==="
sed -n '55,95p' "$FILE"
echo "=== Search: any layout.weight import ==="
rg -n '^import .*\.layout\.weight' "$FILE" || true
echo "=== Search: any weight( usage in file ==="
rg -n 'weight\(' "$FILE" || true
echo "=== Search: Modifier.weight( usage ==="
rg -n 'Modifier\.weight\(' "$FILE" || trueRepository: tejpratap46/AndroidVideoMotion
Length of output: 5157
Add missing weight import for Modifier.weight(1f)
MotionEditorScreen.kt uses Modifier.weight(1f) (around line 67) but doesn’t import androidx.compose.foundation.layout.weight, causing a compilation failure.
Proposed patch
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.statusBarsPadding
+import androidx.compose.foundation.layout.weight📝 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.
Suggested change
| import androidx.compose.foundation.layout.height | |
| import androidx.compose.foundation.layout.navigationBarsPadding | |
| import androidx.compose.foundation.layout.padding | |
| import androidx.compose.foundation.layout.statusBarsPadding | |
| import androidx.compose.foundation.layout.height | |
| import androidx.compose.foundation.layout.navigationBarsPadding | |
| import androidx.compose.foundation.layout.padding | |
| import androidx.compose.foundation.layout.statusBarsPadding | |
| import androidx.compose.foundation.layout.weight |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@modules/motion-video-editor/src/main/java/com/tejpratapsingh/motioneditor/ui/MotionEditorScreen.kt`
around lines 8 - 11, Add the missing import for Modifier.weight by importing
androidx.compose.foundation.layout.weight; locate the usage of
Modifier.weight(1f) in MotionEditorScreen (the composable function
MotionEditorScreen) and add the androidx.compose.foundation.layout.weight import
alongside the other layout imports so the file compiles.
Comment on lines
14
to
+15
| import androidx.compose.foundation.layout.padding | ||
| import androidx.compose.foundation.shape.RoundedCornerShape |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify weight is used and the import is missing
rg -n 'Modifier\.weight\(' modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt
rg -n '^import androidx\.compose\.foundation\.layout\.weight$' modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 56
🏁 Script executed:
#!/bin/bash
FILE="modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt"
# Show imports at the top
echo "=== Imports ==="
sed -n '1,60p' "$FILE"
# Show the region around the reported usage
echo
echo "=== Around lines 160-210 ==="
nl -ba "$FILE" | sed -n '150,210p'
# Search for weight usage more generally (covers ".weight(" and "Modifier.weight(" forms)
echo
echo "=== weight usage matches ==="
rg -n '\.weight\s*\(' "$FILE" || true
rg -n 'Modifier\.weight\s*\(' "$FILE" || true
# Confirm whether the weight import exists (allow whitespace variations)
echo
echo "=== weight import matches ==="
rg -n 'import\s+androidx\.compose\.foundation\.layout\.weight(\s*;)?' "$FILE" || trueRepository: tejpratap46/AndroidVideoMotion
Length of output: 2875
🏁 Script executed:
#!/bin/bash
FILE="modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt"
echo "=== weight import (if any) ==="
rg -n 'import\s+androidx\.compose\.foundation\.layout\.weight(\s*;)?' "$FILE" || true
echo
echo "=== Code around .weight(1f) ==="
# Use cat -n (instead of nl) to show line numbers
cat -n "$FILE" | sed -n '165,190p'Repository: tejpratap46/AndroidVideoMotion
Length of output: 1204
🏁 Script executed:
#!/bin/bash
FILE="modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt"
echo "=== Any foundation.layout wildcard import? ==="
rg -n 'import\s+androidx\.compose\.foundation\.layout\.\*$' "$FILE" || true
echo
echo "=== Any other import containing 'weight'? ==="
rg -n 'import\s+.*weight' "$FILE" || true
echo
echo "=== Any explicit fully-qualified weight call? ==="
rg -n 'androidx\.compose\.foundation\.layout\.weight' "$FILE" || trueRepository: tejpratap46/AndroidVideoMotion
Length of output: 219
Add missing weight import for Modifier.weight(1f)
Modifier.weight(1f) is used in the preview Box modifier (lines ~175-178), but androidx.compose.foundation.layout.weight isn’t imported, leading to an unresolved reference build failure.
Proposed patch
import androidx.compose.foundation.layout.padding
+import androidx.compose.foundation.layout.weight
import androidx.compose.foundation.shape.RoundedCornerShape📝 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.
Suggested change
| import androidx.compose.foundation.layout.padding | |
| import androidx.compose.foundation.shape.RoundedCornerShape | |
| import androidx.compose.foundation.layout.padding | |
| import androidx.compose.foundation.layout.weight | |
| import androidx.compose.foundation.shape.RoundedCornerShape |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt`
around lines 14 - 15, The preview's Box uses Modifier.weight(1f) but the import
for weight is missing; add the missing import for
androidx.compose.foundation.layout.weight so Modifier.weight(1f) resolves (look
for the preview Box/Modifier.weight usage around the preview function in
MotionVideoPlayerCompose.kt and add the import alongside the other
androidx.compose.foundation.layout imports).
| startFrame = 0, | ||
| endFrame = 100 | ||
| ) | ||
| audio(File(context.cacheDir, "test.mp3"), startFrame = 0, endFrame = 100) |
Contributor
There was a problem hiding this comment.
Create the test audio file explicitly before passing it to audio(...).
Line 34 references File(context.cacheDir, "test.mp3") without creating it. This makes the test implicitly depend on provider behavior and can become brittle if file existence checks are added.
Proposed fix
+ val testAudio = File.createTempFile("test_", ".mp3", appContext.cacheDir).apply {
+ writeBytes(byteArrayOf(0x00))
+ }
+
val template = motionTemplate("Test Template") {
parameters {
string("title")
}
content {
val title = data.getString("title") ?: "Default"
popUpTextView(
text = title,
startFrame = 0,
endFrame = 100
)
- audio(File(context.cacheDir, "test.mp3"), startFrame = 0, endFrame = 100)
+ audio(testAudio, startFrame = 0, endFrame = 100)
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@modules/templates/src/androidTest/java/com/tejpratapsingh/motionlib/templates/sdui/MotionTemplateSDUIProviderTest.kt`
at line 34, The test is passing File(context.cacheDir, "test.mp3") to audio(...)
without creating it first; update MotionTemplateSDUIProviderTest so before
calling audio(...) you explicitly create the test file (e.g., call
createNewFile() or writeBytes(...) on File(context.cacheDir, "test.mp3")) and
verify it exists, then pass that File to audio(...); reference the File(...)
construction and the audio(...) call so the change is localized to that test and
does not alter production code.
0eb49b2 to
9618341
Compare
* motion-video-editor: implement `MotionTimeline` and `MotionEditorScreen` components * motion-video-editor: add `TimelineUtils` to map SDUI views to timeline tracks * lyrics-maker: add `VideoEditor` screen to `AppNavHost` and update navigation flow * motion-video-player: add frame-stepping controls and update player UI layout * build: configure `motion-video-editor` module and project dependencies
tejpratap46
force-pushed
the
feat/motion-video-editor
branch
from
9618341 to
f36afd1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MotionTimelineandMotionEditorScreencomponentsTimelineUtilsto map SDUI views to timeline tracksVideoEditorscreen toAppNavHostand update navigation flowmotion-video-editormodule and project dependenciesRef: [SDUI Video Editor] Init SDUI Video Editor in a new Module. #52
Summary by CodeRabbit