Refactor lyrics-maker Compose files into use-case packages#70
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR reorganizes the Compose presentation layer by centralizing navigation, modularizing the Projects UI into route/screen/card/extractor components, extracting lyrics selection models and draggable handles, and moving preview/common components into feature packages. ChangesCompose Presentation Layer Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 docstrings
🧪 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 |
![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 successfully refactors the lyrics-maker Compose architecture by splitting a monolithic file into well-organized, feature-based packages. The refactoring improves code maintainability and navigability without introducing functional defects.
Key Changes
- Extracted
Screensealed class into dedicatednavigation/Screen.kt - Organized Compose components into logical packages (
common,navigation,projects,lyrics,search,templates,details) - Updated all import statements across affected files to reference new package structure
- Maintained backward compatibility and functional equivalence
Assessment
✅ No blocking issues found - All changes are structural refactoring with correct import updates. The code maintains its existing functionality while improving organization and maintainability.
The refactoring follows established Android/Kotlin best practices for feature-based package organization in Compose applications.
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.
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
motion-lib | ca0ac20 | Commit Preview URL Branch Preview URL |
Jun 08 2026, 06:49 PM |
![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 refactors the Compose UI layer by modularizing files into dedicated subpackages (such as navigation, details, lyrics, projects, common, templates, and search) and splitting large files into smaller, more maintainable components. The review feedback highlights critical improvements for the newly introduced components: first, addressing potential stale lambda executions in DragHandle and StartDragHandle by wrapping drag callbacks in rememberUpdatedState within the non-restarting pointerInput(Unit) blocks; second, using toIntOrNull() instead of toInt() when parsing lyric keys in LyricsSelectionModels.kt to safely handle potential formatting changes and prevent NumberFormatException crashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| onDragEnd: () -> Unit, | ||
| onDragCancel: () -> Unit, | ||
| ) { | ||
| var selfTopInRoot by remember { mutableFloatStateOf(0f) } |
There was a problem hiding this comment.
Using pointerInput(Unit) creates a gesture detector that never restarts because its key is Unit. Since the drag callbacks (onDragStart, onDrag, etc.) are captured inside this non-restarting block, any updates to these callbacks from parent recompositions will be ignored, leading to stale lambda execution. To fix this, use rememberUpdatedState for the callbacks so the gesture detector always accesses the latest instances without interrupting active drag gestures.
var selfTopInRoot by remember { mutableFloatStateOf(0f) }
val currentOnDragStart by androidx.compose.runtime.rememberUpdatedState(onDragStart)
val currentOnDrag by androidx.compose.runtime.rememberUpdatedState(onDrag)
val currentOnDragEnd by androidx.compose.runtime.rememberUpdatedState(onDragEnd)
val currentOnDragCancel by androidx.compose.runtime.rememberUpdatedState(onDragCancel)
Comment on lines
+69
to
+91
| }.pointerInput(Unit) { | ||
| detectDragGestures( | ||
| onDragStart = { localOffset -> | ||
| val startY = selfTopInRoot + localOffset.y | ||
| autoScroll.isDragging = true | ||
| autoScroll.pointerYInRoot = startY | ||
| onDragStart(startY) | ||
| }, | ||
| onDrag = { _, dragAmount -> | ||
| val newY = autoScroll.pointerYInRoot + dragAmount.y | ||
| autoScroll.pointerYInRoot = newY | ||
| onDrag(newY) | ||
| }, | ||
| onDragEnd = { | ||
| autoScroll.isDragging = false | ||
| onDragEnd() | ||
| }, | ||
| onDragCancel = { | ||
| autoScroll.isDragging = false | ||
| onDragCancel() | ||
| }, | ||
| ) | ||
| }.background(color.copy(alpha = if (isBeingDragged) 0.8f else 0.6f)) |
There was a problem hiding this comment.
Update the pointerInput block to use the updated state callbacks to prevent stale lambda execution during drag gestures.
}.pointerInput(Unit) {
detectDragGestures(
onDragStart = { localOffset ->
val startY = selfTopInRoot + localOffset.y
autoScroll.isDragging = true
autoScroll.pointerYInRoot = startY
currentOnDragStart(startY)
},
onDrag = { _, dragAmount ->
val newY = autoScroll.pointerYInRoot + dragAmount.y
autoScroll.pointerYInRoot = newY
currentOnDrag(newY)
},
onDragEnd = {
autoScroll.isDragging = false
currentOnDragEnd()
},
onDragCancel = {
autoScroll.isDragging = false
currentOnDragCancel()
},
)
}.background(color.copy(alpha = if (isBeingDragged) 0.8f else 0.6f))| onDragEnd: () -> Unit, | ||
| onDragCancel: () -> Unit, | ||
| ) { | ||
| var selfTopInRoot by remember { mutableFloatStateOf(0f) } |
There was a problem hiding this comment.
Using pointerInput(Unit) creates a gesture detector that never restarts because its key is Unit. Since the drag callbacks (onDragStart, onDrag, etc.) are captured inside this non-restarting block, any updates to these callbacks from parent recompositions will be ignored, leading to stale lambda execution. To fix this, use rememberUpdatedState for the callbacks so the gesture detector always accesses the latest instances without interrupting active drag gestures.
var selfTopInRoot by remember { mutableFloatStateOf(0f) }
val currentOnDragStart by androidx.compose.runtime.rememberUpdatedState(onDragStart)
val currentOnDrag by androidx.compose.runtime.rememberUpdatedState(onDrag)
val currentOnDragEnd by androidx.compose.runtime.rememberUpdatedState(onDragEnd)
val currentOnDragCancel by androidx.compose.runtime.rememberUpdatedState(onDragCancel)
Comment on lines
+110
to
+132
| .pointerInput(Unit) { | ||
| detectDragGestures( | ||
| onDragStart = { localOffset -> | ||
| val startY = selfTopInRoot + localOffset.y | ||
| autoScroll.isDragging = true | ||
| autoScroll.pointerYInRoot = startY | ||
| onDragStart(startY) | ||
| }, | ||
| onDrag = { _, dragAmount -> | ||
| val newY = autoScroll.pointerYInRoot + dragAmount.y | ||
| autoScroll.pointerYInRoot = newY | ||
| onDrag(newY) | ||
| }, | ||
| onDragEnd = { | ||
| autoScroll.isDragging = false | ||
| onDragEnd() | ||
| }, | ||
| onDragCancel = { | ||
| autoScroll.isDragging = false | ||
| onDragCancel() | ||
| }, | ||
| ) | ||
| }.padding(vertical = 6.dp), |
There was a problem hiding this comment.
Update the pointerInput block to use the updated state callbacks to prevent stale lambda execution during drag gestures.
.pointerInput(Unit) {
detectDragGestures(
onDragStart = { localOffset ->
val startY = selfTopInRoot + localOffset.y
autoScroll.isDragging = true
autoScroll.pointerYInRoot = startY
currentOnDragStart(startY)
},
onDrag = { _, dragAmount ->
val newY = autoScroll.pointerYInRoot + dragAmount.y
autoScroll.pointerYInRoot = newY
currentOnDrag(newY)
},
onDragEnd = {
autoScroll.isDragging = false
currentOnDragEnd()
},
onDragCancel = {
autoScroll.isDragging = false
currentOnDragCancel()
},
)
}.padding(vertical = 6.dp),
Comment on lines
+141
to
+146
| if (relativeY >= itemTop && relativeY <= itemBottom) { | ||
| return item.key | ||
| .toString() | ||
| .removePrefix("lyric_") | ||
| .toInt() | ||
| } |
There was a problem hiding this comment.
Using .toInt() directly on the parsed key can throw a NumberFormatException if the key format changes or if non-integer keys starting with "lyric_" are introduced. Use .toIntOrNull() to safely handle parsing and avoid potential crashes.
Suggested change
| if (relativeY >= itemTop && relativeY <= itemBottom) { | |
| return item.key | |
| .toString() | |
| .removePrefix("lyric_") | |
| .toInt() | |
| } | |
| if (relativeY >= itemTop && relativeY <= itemBottom) { | |
| return item.key | |
| .toString() | |
| .removePrefix("lyric_") | |
| .toIntOrNull() | |
| } |
Comment on lines
+151
to
+156
| closestIndex = | ||
| item.key | ||
| .toString() | ||
| .removePrefix("lyric_") | ||
| .toInt() | ||
| } |
There was a problem hiding this comment.
Using .toInt() directly on the parsed key can throw a NumberFormatException if the key format changes or if non-integer keys starting with "lyric_" are introduced. Use .toIntOrNull() to safely handle parsing and avoid potential crashes.
Suggested change
| closestIndex = | |
| item.key | |
| .toString() | |
| .removePrefix("lyric_") | |
| .toInt() | |
| } | |
| closestIndex = | |
| item.key | |
| .toString() | |
| .removePrefix("lyric_") | |
| .toIntOrNull() |
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
sdui | ca0ac20 | Commit Preview URL Branch Preview URL |
Jun 08 2026, 06:50 PM |
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
lyrics | ca0ac20 | Commit Preview URL Branch Preview URL |
Jun 08 2026, 06:50 PM |
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
modules/lyrics-maker/src/androidTest/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsScreenTest.kt (6)
150-153:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing required
onEditClickparameter in test.🐛 Proposed fix
composeTestRule.setContent { ProjectDetailsScreen( project = project, onBackClick = {}, + onEditClick = {}, onShareClick = { sharedProject = it }, ) }🤖 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/androidTest/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsScreenTest.kt` around lines 150 - 153, The test is calling the ProjectDetailsScreen composable without the required onEditClick parameter; update the test instantiation in ProjectDetailsScreenTest to include an onEditClick lambda (e.g., onEditClick = {} or capture a variable if you need to assert behavior) alongside the existing onBackClick and onShareClick parameters so the composable signature matches and the test compiles.
186-190:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing required
onEditClickparameter in test.🐛 Proposed fix
composeTestRule.setContent { ProjectDetailsScreen( project = project, onBackClick = { backClicked = true }, + onEditClick = {}, onShareClick = {}, ) }🤖 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/androidTest/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsScreenTest.kt` around lines 186 - 190, The test invokes ProjectDetailsScreen without the required onEditClick parameter; update the ProjectDetailsScreen call in ProjectDetailsScreenTest.kt to pass an onEditClick lambda (e.g., set a Boolean like editClicked = true or a noop) so the composable receives the required callback; reference the ProjectDetailsScreen invocation and add the onEditClick argument alongside onBackClick and onShareClick.
72-76:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing required
onEditClickparameter in test.The
ProjectDetailsScreensignature requires anonEditClick: (MotionProject) -> Unitparameter (see ProjectDetailsScreen.kt lines 54), but this test call omits it. This code will not compile.🐛 Proposed fix to add the missing parameter
composeTestRule.setContent { ProjectDetailsScreen( project = project, onBackClick = {}, + onEditClick = {}, onShareClick = {}, ) }Apply similar fixes to all other test methods in this file.
🤖 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/androidTest/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsScreenTest.kt` around lines 72 - 76, The test call to ProjectDetailsScreen is missing the required onEditClick parameter; update the test invocations of ProjectDetailsScreen to include onEditClick with a matching signature (e.g., add onEditClick = { _: MotionProject -> } or onEditClick = {} where a MotionProject parameter is accepted), and apply the same change to all other test methods in this test file so the calls match the ProjectDetailsScreen(onBackClick, onShareClick, onEditClick) signature.
133-137:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing required
onEditClickparameter in test.🐛 Proposed fix
composeTestRule.setContent { ProjectDetailsScreen( project = project, onBackClick = {}, + onEditClick = {}, onShareClick = {}, ) }🤖 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/androidTest/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsScreenTest.kt` around lines 133 - 137, The test invocation of ProjectDetailsScreen is missing the required onEditClick parameter; update the call in ProjectDetailsScreenTest to pass a suitable lambda (e.g., {}) for onEditClick so the function signature matches ProjectDetailsScreen(project = project, onBackClick = {}, onShareClick = {}, onEditClick = {}); ensure you add the onEditClick argument wherever ProjectDetailsScreen is constructed in the test to satisfy the required parameter.
170-174:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing required
onEditClickparameter in test.🐛 Proposed fix
composeTestRule.setContent { ProjectDetailsScreen( project = project, onBackClick = {}, + onEditClick = {}, onShareClick = {}, ) }🤖 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/androidTest/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsScreenTest.kt` around lines 170 - 174, The test invocation of ProjectDetailsScreen is missing the required onEditClick parameter; update the call in ProjectDetailsScreenTest (the ProjectDetailsScreen(...) block) to include a stubbed lambda for onEditClick (e.g., onEditClick = {}), matching the other callbacks (onBackClick, onShareClick) so the composable is called with all required parameters.
91-95:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing required
onEditClickparameter in test.Same issue as the previous test:
onEditClickis required but not provided.🐛 Proposed fix
composeTestRule.setContent { ProjectDetailsScreen( project = project, onBackClick = {}, + onEditClick = {}, onShareClick = {}, ) }🤖 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/androidTest/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsScreenTest.kt` around lines 91 - 95, The test in ProjectDetailsScreenTest.kt is calling ProjectDetailsScreen without the required onEditClick parameter; update the ProjectDetailsScreen invocation to include an onEditClick argument (e.g., onEditClick = {} or a test lambda/mock) alongside the existing onBackClick and onShareClick so the composable is constructed with all required parameters.
🧹 Nitpick comments (7)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/StartDragHandle.kt (1)
81-85: 💤 Low valueClarify the position tracking comment.
The comment on lines 82-83 is somewhat confusing. It suggests conditional updating ("only update when NOT dragging") but the code unconditionally updates
selfTopInRootby subtractingvisualOffset.Consider rephrasing for clarity:
// Calculate natural position by subtracting the current visual offset // to avoid feedback loops during drag. selfTopInRoot = coords.positionInRoot().y - visualOffset🤖 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/lyrics/StartDragHandle.kt` around lines 81 - 85, Update the confusing comment inside the onGloballyPositioned lambda in StartDragHandle.kt to accurately describe what's happening: state selfTopInRoot is being set to the view's natural Y position adjusted by the current visualOffset to avoid feedback loops during dragging; reference the variables selfTopInRoot and visualOffset and the onGloballyPositioned callback so the comment clearly reads something like "Calculate natural position by subtracting the current visual offset to avoid feedback loops during drag."modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/DragHandle.kt (1)
37-106: 🏗️ Heavy liftConsider extracting common drag handle logic.
DragHandleandStartDragHandleshare significant implementation:
- Identical
selfTopInRoottracking andvisualOffsetcomputation- Identical
onGloballyPositionedcoordinate correction logic- Identical
detectDragGesturesstructure withautoScrollstate coordinationThe main differences are the move-mode toggle in
StartDragHandleand different label rendering. Consider refactoring to reduce duplication:Option 1: Extract a
BaseDragHandlecomposable with slots for custom content (toggle button, label, etc.)Option 2: Extract the drag gesture logic into a custom
Modifier.draggableHandle()extension that encapsulates position tracking and auto-scroll coordination.Since this PR focuses on splitting files, addressing this duplication could be deferred to a follow-up refactoring task.
🤖 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/lyrics/DragHandle.kt` around lines 37 - 106, DragHandle and StartDragHandle duplicate the same position tracking and gesture logic (selfTopInRoot, visualOffset, onGloballyPositioned and detectDragGestures coordinating with AutoScrollState), so extract that shared behavior into a single reusable unit: either create a BaseDragHandle composable that takes a content slot (for differing label/toggle UI) and parameters like label, color, autoScroll, isBeingDragged, livePointerYInRoot, onDragStart/onDrag/onDragEnd/onDragCancel and implements the shared state and gestures, or implement a Modifier.draggableHandle(autoScroll, isBeingDragged, livePointerYInRoot, onDragStart, onDrag, onDragEnd, onDragCancel) extension that encapsulates selfTopInRoot, visualOffset, onGloballyPositioned and detectDragGestures; then update DragHandle and StartDragHandle to use the new BaseDragHandle or attach the new modifier and provide only their unique UI bits (toggle/label).modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/LyricsSelectionModels.kt (2)
166-166: 💤 Low valueRemove unnecessary parentheses.
The parentheses around
frames / fpsare unnecessary and can be removed for cleaner code.♻️ Proposed fix
- val totalSecs = (frames / fps) + val totalSecs = frames / fps🤖 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/lyrics/LyricsSelectionModels.kt` at line 166, Remove the unnecessary parentheses around the division expression when computing totalSecs: change the assignment of the variable totalSecs (currently val totalSecs = (frames / fps)) to use val totalSecs = frames / fps so the expression is cleaner; locate this in the block where totalSecs, frames and fps are defined (symbol names: totalSecs, frames, fps) and update accordingly.Source: Linters/SAST tools
108-160: ⚡ Quick winConsider more robust key parsing.
The function relies on string manipulation (
.removePrefix("lyric_").toInt()) to extract lyric indices from lazy list keys. If the key format changes or contains unexpected values, this could fail silently with aNumberFormatException.Consider using a more structured approach, such as:
- Storing the index as metadata in the key object (e.g., a data class key)
- Adding try-catch with fallback behavior
- Adding key format validation
However, since the caller clamps the result and this is internal code with controlled key formats, the current approach is acceptable for now.
🤖 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/lyrics/LyricsSelectionModels.kt` around lines 108 - 160, The key-parsing in findLyricIndexAt uses item.key.toString().removePrefix("lyric_").toInt() which can throw NumberFormatException if the key changes; update findLyricIndexAt to validate and safely parse keys: check that item.key.toString() startsWith "lyric_" and the suffix is a valid integer (use try/catch or Integer.parseInt with validation) and skip any invalid keys or fall back to nearest valid index, ensuring you never propagate a throwable from the parsing step.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectsScreen.kt (1)
103-103: ⚡ Quick winExtract hardcoded strings to string resources.
The sort UI contains several hardcoded strings:
- Line 103: contentDescription for accessibility
- Line 106: "Sort" button label
- Lines 116, 128: "Created On" and "Updated On" menu items
Line 86 correctly uses
stringResource(R.string.app_name), but the sort menu strings are hardcoded. For proper i18n/l10n support, these should be extracted to string resources.🌍 Suggested approach
Add to
strings.xml:<string name="sort">Sort</string> <string name="sort_created_on">Created On</string> <string name="sort_updated_on">Updated On</string> <string name="content_description_sort">Sort projects</string>Then update the composable:
Icon( Icons.AutoMirrored.Rounded.Sort, contentDescription = stringResource(R.string.content_description_sort), ) Text( stringResource(R.string.sort), style = MaterialTheme.typography.labelLarge, fontWeight = FontWeight.SemiBold, ) // ... in menu items: text = { Text(stringResource(R.string.sort_created_on)) } text = { Text(stringResource(R.string.sort_updated_on)) }Also applies to: 106-106, 116-116, 128-128
🤖 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/projects/ProjectsScreen.kt` at line 103, The UI has hardcoded strings in ProjectsScreen composable (contentDescription on the Sort Icon, the "Sort" Text label, and the "Created On"/"Updated On" menu item texts); extract these into string resources (e.g., sort, sort_created_on, sort_updated_on, content_description_sort) in strings.xml and replace the hardcoded literals with stringResource(...) calls where contentDescription (Sort Icon), the Text label (Sort), and the menu item text lambdas are defined so the composable uses localized resources instead of literals.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/CreateNewProjectCard.kt (1)
69-69: ⚡ Quick winExtract hardcoded strings to string resources.
The card contains hardcoded UI text and accessibility strings:
- Line 69: contentDescription for the icon
- Line 76: "New Project" label
For proper i18n/l10n support and consistency with other parts of the codebase (like the app name on line 86 of ProjectsScreen), these should be extracted to string resources.
🌍 Suggested approach
Add to
strings.xml:<string name="new_project">New Project</string> <string name="content_description_create_new_project">Create new project</string>Then update the composable:
Icon( imageVector = Icons.Rounded.Add, contentDescription = stringResource(R.string.content_description_create_new_project), tint = MaterialTheme.colorScheme.primary, modifier = Modifier.size(28.dp), ) // ... Text( text = stringResource(R.string.new_project), style = MaterialTheme.typography.titleSmall, color = MaterialTheme.colorScheme.primary, fontWeight = FontWeight.SemiBold, )Also applies to: 76-76
🤖 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/projects/CreateNewProjectCard.kt` at line 69, The CreateNewProjectCard composable has hardcoded UI and accessibility strings (Icon contentDescription and the "New Project" Text) — extract these to string resources (e.g., new_project and content_description_create_new_project) in strings.xml and replace the hardcoded literals in CreateNewProjectCard by calling stringResource(R.string.new_project) for the Text and stringResource(R.string.content_description_create_new_project) for the Icon's contentDescription so the composable uses localized resources instead of hardcoded strings.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectThumbnailExtractor.kt (1)
5-5: 💤 Low valueRemove redundant import.
The import
java.lang.Exceptionis unnecessary in Kotlin, asExceptionis available without an explicit import.🧹 Suggested cleanup
import android.graphics.Bitmap import android.media.MediaMetadataRetriever -import java.lang.Exception🤖 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/projects/ProjectThumbnailExtractor.kt` at line 5, Remove the redundant explicit import "java.lang.Exception" from ProjectThumbnailExtractor.kt; Exception is available by default in Kotlin so delete the import statement to clean up unused imports and keep the file tidy.
🤖 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/projects/MotionProjectUpdatedLabel.kt`:
- Around line 5-13: The updatedLabel() function can produce negative diffs when
MotionProject.updated is in the future; clamp or handle that case by computing
diff = max(0, System.currentTimeMillis() - updated) (or return "Just now" when
updated > now) before the when-expression so the label never shows negative
values; update the MotionProject.updatedLabel() implementation to use this
guarded diff and keep the existing time thresholds and formatting.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectCard.kt`:
- Around line 65-81: The thumbnail extraction inside the LaunchedEffect (the
withContext(Dispatchers.IO) block calling extractFirstFrame) can hang; wrap the
extraction call in a coroutine timeout (e.g., withTimeoutOrNull) to bound how
long extractFirstFrame is allowed to run, treat a timeout as a null result and
avoid inserting into ThumbnailCache if timed out, and keep the UI update
(setting thumbnail) only when a non-null bitmap is returned; update the
LaunchedEffect/withContext scope around extractFirstFrame and ensure you handle
cancellation and IO cleanup appropriately.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectThumbnailExtractor.kt`:
- Around line 7-23: Make extractFirstFrame safe against indefinite blocking by
making it suspend and adding a cancellation-aware timeout around the
MediaMetadataRetriever call: change fun extractFirstFrame(videoPath: String):
Bitmap? to suspend fun extractFirstFrame(videoPath: String, timeoutMillis: Long
= 5_000): Bitmap? and run the retrieval inside withContext(Dispatchers.IO) {
withTimeoutOrNull(timeoutMillis) { retriever.setDataSource(videoPath);
retriever.getScaledFrameAtTime(0, MediaMetadataRetriever.OPTION_CLOSEST_SYNC,
300, 300) } }, ensuring retriever.release() remains in finally and that the
function returns null on timeout/cancellation; update the ProjectCard.kt call
site to call the suspend variant if needed.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/templates/TemplatePreviewItem.kt`:
- Around line 36-42: The produceState block that initializes motionVideoProducer
currently wraps createLyricsVideoPreviewProducer(...) in
withContext(Dispatchers.Default), which can block the Default pool because
MotionImageView/ ImageUtil.fetchBitmap perform IO and may block; change the
dispatcher to withContext(Dispatchers.IO) so the produceState initialization
runs on the IO dispatcher. Locate the produceState usage that sets
motionVideoProducer in TemplatePreviewItem.kt and replace Dispatchers.Default
with Dispatchers.IO when calling createLyricsVideoPreviewProducer to avoid
blocking threads used by MotionImageView/ImageUtil.fetchBitmap.
---
Outside diff comments:
In
`@modules/lyrics-maker/src/androidTest/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsScreenTest.kt`:
- Around line 150-153: The test is calling the ProjectDetailsScreen composable
without the required onEditClick parameter; update the test instantiation in
ProjectDetailsScreenTest to include an onEditClick lambda (e.g., onEditClick =
{} or capture a variable if you need to assert behavior) alongside the existing
onBackClick and onShareClick parameters so the composable signature matches and
the test compiles.
- Around line 186-190: The test invokes ProjectDetailsScreen without the
required onEditClick parameter; update the ProjectDetailsScreen call in
ProjectDetailsScreenTest.kt to pass an onEditClick lambda (e.g., set a Boolean
like editClicked = true or a noop) so the composable receives the required
callback; reference the ProjectDetailsScreen invocation and add the onEditClick
argument alongside onBackClick and onShareClick.
- Around line 72-76: The test call to ProjectDetailsScreen is missing the
required onEditClick parameter; update the test invocations of
ProjectDetailsScreen to include onEditClick with a matching signature (e.g., add
onEditClick = { _: MotionProject -> } or onEditClick = {} where a MotionProject
parameter is accepted), and apply the same change to all other test methods in
this test file so the calls match the ProjectDetailsScreen(onBackClick,
onShareClick, onEditClick) signature.
- Around line 133-137: The test invocation of ProjectDetailsScreen is missing
the required onEditClick parameter; update the call in ProjectDetailsScreenTest
to pass a suitable lambda (e.g., {}) for onEditClick so the function signature
matches ProjectDetailsScreen(project = project, onBackClick = {}, onShareClick =
{}, onEditClick = {}); ensure you add the onEditClick argument wherever
ProjectDetailsScreen is constructed in the test to satisfy the required
parameter.
- Around line 170-174: The test invocation of ProjectDetailsScreen is missing
the required onEditClick parameter; update the call in ProjectDetailsScreenTest
(the ProjectDetailsScreen(...) block) to include a stubbed lambda for
onEditClick (e.g., onEditClick = {}), matching the other callbacks (onBackClick,
onShareClick) so the composable is called with all required parameters.
- Around line 91-95: The test in ProjectDetailsScreenTest.kt is calling
ProjectDetailsScreen without the required onEditClick parameter; update the
ProjectDetailsScreen invocation to include an onEditClick argument (e.g.,
onEditClick = {} or a test lambda/mock) alongside the existing onBackClick and
onShareClick so the composable is constructed with all required parameters.
---
Nitpick comments:
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/DragHandle.kt`:
- Around line 37-106: DragHandle and StartDragHandle duplicate the same position
tracking and gesture logic (selfTopInRoot, visualOffset, onGloballyPositioned
and detectDragGestures coordinating with AutoScrollState), so extract that
shared behavior into a single reusable unit: either create a BaseDragHandle
composable that takes a content slot (for differing label/toggle UI) and
parameters like label, color, autoScroll, isBeingDragged, livePointerYInRoot,
onDragStart/onDrag/onDragEnd/onDragCancel and implements the shared state and
gestures, or implement a Modifier.draggableHandle(autoScroll, isBeingDragged,
livePointerYInRoot, onDragStart, onDrag, onDragEnd, onDragCancel) extension that
encapsulates selfTopInRoot, visualOffset, onGloballyPositioned and
detectDragGestures; then update DragHandle and StartDragHandle to use the new
BaseDragHandle or attach the new modifier and provide only their unique UI bits
(toggle/label).
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/LyricsSelectionModels.kt`:
- Line 166: Remove the unnecessary parentheses around the division expression
when computing totalSecs: change the assignment of the variable totalSecs
(currently val totalSecs = (frames / fps)) to use val totalSecs = frames / fps
so the expression is cleaner; locate this in the block where totalSecs, frames
and fps are defined (symbol names: totalSecs, frames, fps) and update
accordingly.
- Around line 108-160: The key-parsing in findLyricIndexAt uses
item.key.toString().removePrefix("lyric_").toInt() which can throw
NumberFormatException if the key changes; update findLyricIndexAt to validate
and safely parse keys: check that item.key.toString() startsWith "lyric_" and
the suffix is a valid integer (use try/catch or Integer.parseInt with
validation) and skip any invalid keys or fall back to nearest valid index,
ensuring you never propagate a throwable from the parsing step.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/StartDragHandle.kt`:
- Around line 81-85: Update the confusing comment inside the
onGloballyPositioned lambda in StartDragHandle.kt to accurately describe what's
happening: state selfTopInRoot is being set to the view's natural Y position
adjusted by the current visualOffset to avoid feedback loops during dragging;
reference the variables selfTopInRoot and visualOffset and the
onGloballyPositioned callback so the comment clearly reads something like
"Calculate natural position by subtracting the current visual offset to avoid
feedback loops during drag."
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/CreateNewProjectCard.kt`:
- Line 69: The CreateNewProjectCard composable has hardcoded UI and
accessibility strings (Icon contentDescription and the "New Project" Text) —
extract these to string resources (e.g., new_project and
content_description_create_new_project) in strings.xml and replace the hardcoded
literals in CreateNewProjectCard by calling stringResource(R.string.new_project)
for the Text and stringResource(R.string.content_description_create_new_project)
for the Icon's contentDescription so the composable uses localized resources
instead of hardcoded strings.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectsScreen.kt`:
- Line 103: The UI has hardcoded strings in ProjectsScreen composable
(contentDescription on the Sort Icon, the "Sort" Text label, and the "Created
On"/"Updated On" menu item texts); extract these into string resources (e.g.,
sort, sort_created_on, sort_updated_on, content_description_sort) in strings.xml
and replace the hardcoded literals with stringResource(...) calls where
contentDescription (Sort Icon), the Text label (Sort), and the menu item text
lambdas are defined so the composable uses localized resources instead of
literals.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectThumbnailExtractor.kt`:
- Line 5: Remove the redundant explicit import "java.lang.Exception" from
ProjectThumbnailExtractor.kt; Exception is available by default in Kotlin so
delete the import statement to clean up unused imports and keep the file tidy.
🪄 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: e110c5e7-c86a-4577-81a3-fe98fed97b80
📒 Files selected for processing (26)
modules/lyrics-maker/src/androidTest/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsScreenTest.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectsScreenCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/common/GradientText.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/common/GradientTextPreview.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/details/ProjectDetailsScreen.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/DragHandle.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/LyricRow.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/LyricsSelectionModels.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/StartDragHandle.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/SyncedLyricsSelector.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/navigation/AppNavHost.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/navigation/Screen.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/CreateNewProjectCard.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/DeleteConfirmationDialog.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/MotionProjectUpdatedLabel.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectCard.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectThumbnailExtractor.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectsRoute.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectsScreen.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ThumbnailCache.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/search/SearchScreen.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/templates/LyricsTemplateSelector.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/templates/TemplatePreviewItem.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/ProjectsViewModel.ktmodules/lyrics-maker/src/test/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ScreenTest.kt
💤 Files with no reviewable changes (1)
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectsScreenCompose.kt
Comment on lines
+5
to
+13
| internal fun MotionProject.updatedLabel(): String { | ||
| val diff = System.currentTimeMillis() - updated | ||
| return when { | ||
| diff < 60_000 -> "Just now" | ||
| diff < 3_600_000 -> "${diff / 60_000}m ago" | ||
| diff < 86_400_000 -> "${diff / 3_600_000}h ago" | ||
| else -> "${diff / 86_400_000}d ago" | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Handle future timestamps to avoid negative time labels.
If updated is greater than System.currentTimeMillis() (possible due to clock adjustments or data issues), diff will be negative and the labels will display confusing values like "-5m ago". Consider adding a guard to handle future timestamps gracefully.
🛡️ Suggested fix
internal fun MotionProject.updatedLabel(): String {
val diff = System.currentTimeMillis() - updated
+ if (diff < 0) return "Just now"
return when {
diff < 60_000 -> "Just now"
diff < 3_600_000 -> "${diff / 60_000}m ago"
diff < 86_400_000 -> "${diff / 3_600_000}h ago"
else -> "${diff / 86_400_000}d ago"
}
}🤖 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/projects/MotionProjectUpdatedLabel.kt`
around lines 5 - 13, The updatedLabel() function can produce negative diffs when
MotionProject.updated is in the future; clamp or handle that case by computing
diff = max(0, System.currentTimeMillis() - updated) (or return "Just now" when
updated > now) before the when-expression so the label never shows negative
values; update the MotionProject.updatedLabel() implementation to use this
guarded diff and keep the existing time thresholds and formatting.
Comment on lines
+65
to
+81
| LaunchedEffect(project.id, project.updated) { | ||
| if (thumbnail == null) { | ||
| withContext(Dispatchers.IO) { | ||
| val projectFile = context.createProjectFile(project) | ||
| if (projectFile.exists()) { | ||
| val extractedBitmap = extractFirstFrame(projectFile.path) | ||
| extractedBitmap?.let { | ||
| ThumbnailCache.removeByPrefix(project.id) | ||
| ThumbnailCache.put("${project.id}_${project.updated}", it) | ||
| withContext(Dispatchers.Main) { | ||
| thumbnail = it | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Consider adding a timeout for thumbnail extraction.
The extractFirstFrame call on Line 70 uses MediaMetadataRetriever, which can block indefinitely if the video file is corrupted or unusually large. While the LaunchedEffect will cancel when the composable leaves the composition, a long-running extraction holds IO thread resources and provides poor UX if the user has a problematic video file.
⏱️ Suggested approach: wrap extraction with timeout
Consider wrapping the extraction in withTimeout or withTimeoutOrNull:
LaunchedEffect(project.id, project.updated) {
if (thumbnail == null) {
withContext(Dispatchers.IO) {
val projectFile = context.createProjectFile(project)
if (projectFile.exists()) {
- val extractedBitmap = extractFirstFrame(projectFile.path)
+ val extractedBitmap = withTimeoutOrNull(5000L) {
+ extractFirstFrame(projectFile.path)
+ }
extractedBitmap?.let {
ThumbnailCache.removeByPrefix(project.id)
ThumbnailCache.put("${project.id}_${project.updated}", it)
withContext(Dispatchers.Main) {
thumbnail = it
}
}
}
}
}
}📝 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
| LaunchedEffect(project.id, project.updated) { | |
| if (thumbnail == null) { | |
| withContext(Dispatchers.IO) { | |
| val projectFile = context.createProjectFile(project) | |
| if (projectFile.exists()) { | |
| val extractedBitmap = extractFirstFrame(projectFile.path) | |
| extractedBitmap?.let { | |
| ThumbnailCache.removeByPrefix(project.id) | |
| ThumbnailCache.put("${project.id}_${project.updated}", it) | |
| withContext(Dispatchers.Main) { | |
| thumbnail = it | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| LaunchedEffect(project.id, project.updated) { | |
| if (thumbnail == null) { | |
| withContext(Dispatchers.IO) { | |
| val projectFile = context.createProjectFile(project) | |
| if (projectFile.exists()) { | |
| val extractedBitmap = withTimeoutOrNull(5000L) { | |
| extractFirstFrame(projectFile.path) | |
| } | |
| extractedBitmap?.let { | |
| ThumbnailCache.removeByPrefix(project.id) | |
| ThumbnailCache.put("${project.id}_${project.updated}", it) | |
| withContext(Dispatchers.Main) { | |
| thumbnail = it | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } |
🤖 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/projects/ProjectCard.kt`
around lines 65 - 81, The thumbnail extraction inside the LaunchedEffect (the
withContext(Dispatchers.IO) block calling extractFirstFrame) can hang; wrap the
extraction call in a coroutine timeout (e.g., withTimeoutOrNull) to bound how
long extractFirstFrame is allowed to run, treat a timeout as a null result and
avoid inserting into ThumbnailCache if timed out, and keep the UI update
(setting thumbnail) only when a non-null bitmap is returned; update the
LaunchedEffect/withContext scope around extractFirstFrame and ensure you handle
cancellation and IO cleanup appropriately.
Comment on lines
+7
to
+23
| fun extractFirstFrame(videoPath: String): Bitmap? { | ||
| val retriever = MediaMetadataRetriever() | ||
| return try { | ||
| retriever.setDataSource(videoPath) | ||
| // Request a smaller frame (e.g., 300px) and use OPTION_CLOSEST_SYNC for speed | ||
| retriever.getScaledFrameAtTime( | ||
| 0, | ||
| MediaMetadataRetriever.OPTION_CLOSEST_SYNC, | ||
| 300, | ||
| 300, | ||
| ) | ||
| } catch (e: Exception) { | ||
| null | ||
| } finally { | ||
| retriever.release() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all callers of extractFirstFrame
rg -n --type=kt 'extractFirstFrame\s*\(' -g '!ProjectThumbnailExtractor.kt'Repository: tejpratap46/AndroidVideoMotion
Length of output: 101
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find Kotlin call sites for extractFirstFrame (excluding its definition file) =="
rg -n --glob '*.kt' 'extractFirstFrame\s*\(' -g '!modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectThumbnailExtractor.kt' || true
echo
echo "== Locate ProjectCard.kt =="
fd ProjectCard.kt modules/lyrics-maker/src/main/java || trueRepository: tejpratap46/AndroidVideoMotion
Length of output: 609
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find Kotlin call sites for extractFirstFrame (excluding its definition file) =="
rg -n --glob '*.kt' 'extractFirstFrame\s*\(' -g '!modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectThumbnailExtractor.kt' || true
echo
echo "== Locate ProjectCard.kt (if present) =="
fd -a ProjectCard.kt modules/lyrics-maker/src/main/java || trueRepository: tejpratap46/AndroidVideoMotion
Length of output: 660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== All extractFirstFrame call sites (excluding definition file) =="
rg -n --glob '*.kt' 'extractFirstFrame\s*\(' modules/lyrics-maker/src/main/java -g '!ProjectThumbnailExtractor.kt' || true
echo
echo "== ProjectCard.kt excerpt around call =="
nl -ba modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectCard.kt | sed -n '45,100p'Repository: tejpratap46/AndroidVideoMotion
Length of output: 436
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm call sites again (excluding definition)
echo "== extractFirstFrame call sites (excluding definition) =="
rg -n --glob '*.kt' 'extractFirstFrame\s*\(' modules/lyrics-maker/src/main/java -g '!ProjectThumbnailExtractor.kt' || true
echo
echo "== ProjectCard.kt lines ~55-90 (numbered via python) =="
python3 - <<'PY'
from pathlib import Path
p = Path("modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectCard.kt")
start, end = 55, 90
lines = p.read_text(encoding="utf-8").splitlines()
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: tejpratap46/AndroidVideoMotion
Length of output: 1821
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== extractFirstFrame call sites across repo (excluding definition file) =="
rg -n --glob '*.kt' 'extractFirstFrame\s*\(' -g '!ProjectThumbnailExtractor.kt'
echo
echo "== Timeout handling keywords in ProjectCard.kt =="
rg -n --glob '*.kt' '(withTimeout|withTimeoutOrNull|timeout)' modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectCard.kt || trueRepository: tejpratap46/AndroidVideoMotion
Length of output: 392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== extractFirstFrame call sites across repo (excluding definition file) =="
rg -n --glob '*.kt' 'extractFirstFrame\s*\(' -g '!ProjectThumbnailExtractor.kt'
echo
echo "== Timeout handling keywords in ProjectCard.kt =="
rg -n --glob '*.kt' '(withTimeout|withTimeoutOrNull|timeout)' modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectCard.kt || trueRepository: tejpratap46/AndroidVideoMotion
Length of output: 392
Fix MediaMetadataRetriever indefinite hang in thumbnail extraction.
extractFirstFrame calls MediaMetadataRetriever.getScaledFrameAtTime(...) without any timeout, and the only call site is ProjectCard.kt (around the extractFirstFrame(projectFile.path) line) where it runs on Dispatchers.IO with no withTimeout/timeout guarding. If retrieval blocks (corrupt/large video), thumbnail loading can stall indefinitely. Add timeout/cancellation handling for this call (ideally inside extractFirstFrame).
🤖 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/projects/ProjectThumbnailExtractor.kt`
around lines 7 - 23, Make extractFirstFrame safe against indefinite blocking by
making it suspend and adding a cancellation-aware timeout around the
MediaMetadataRetriever call: change fun extractFirstFrame(videoPath: String):
Bitmap? to suspend fun extractFirstFrame(videoPath: String, timeoutMillis: Long
= 5_000): Bitmap? and run the retrieval inside withContext(Dispatchers.IO) {
withTimeoutOrNull(timeoutMillis) { retriever.setDataSource(videoPath);
retriever.getScaledFrameAtTime(0, MediaMetadataRetriever.OPTION_CLOSEST_SYNC,
300, 300) } }, ensuring retriever.release() remains in finally and that the
function returns null on timeout/cancellation; update the ProjectCard.kt call
site to call the suspend variant if needed.
tejpratap46
force-pushed
the
codex/create-separate-files-for-each-compose
branch
from
de8bce8 to
0ae91d3
Compare
tejpratap46
force-pushed
the
codex/create-separate-files-for-each-compose
branch
from
0ae91d3 to
ca0ac20
Compare
Contributor
There was a problem hiding this comment.
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/compose/lyrics/SyncedLyricsSelector.kt (1)
231-234:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSide effect called directly in composition violates Compose best practices.
onSelectionChanged(selected)is invoked during composition rather than in aLaunchedEffect. This causes the callback to fire on every recomposition wherelyrics.isNotEmpty(), potentially causing excessive invocations or recomposition loops if the callback triggers upstream state changes.🐛 Proposed fix: move callback to LaunchedEffect
Add this effect near the other
LaunchedEffectblocks (e.g., after line 91) and remove the inline call at line 233:// Notify parent when selection changes LaunchedEffect(selected) { onSelectionChanged(selected) }Then remove line 233:
} HorizontalDivider() - onSelectionChanged(selected) }🤖 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/lyrics/SyncedLyricsSelector.kt` around lines 231 - 234, The direct call to onSelectionChanged(selected) inside the SyncedLyricsSelector composable executes during composition and must be moved into a side-effect; remove the inline invocation and add a LaunchedEffect keyed on selected (e.g., LaunchedEffect(selected)) near the other effects in SyncedLyricsSelector so the callback runs only when selection actually changes; ensure you delete the original call (the line invoking onSelectionChanged(selected)) so the only notifier is the new LaunchedEffect.
🧹 Nitpick comments (3)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/LyricsSelectionModels.kt (1)
162-170: 💤 Low valueRemove unnecessary parentheses.
The parentheses around
frames / fpsare redundant.♻️ Suggested fix
internal fun formatDuration( frames: Int, fps: Int, ): String { - val totalSecs = (frames / fps) + val totalSecs = frames / fps val m = totalSecs / 60 val s = totalSecs % 60 return "$m:${s.toString().padStart(2, '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/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/LyricsSelectionModels.kt` around lines 162 - 170, The expression computing totalSecs in function formatDuration unnecessarily wraps the division in parentheses; update the totalSecs assignment in formatDuration (using the parameters frames and fps) to remove the redundant parentheses so it reads totalSecs = frames / fps, leaving the rest of the function unchanged.Source: Linters/SAST tools
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectCard.kt (2)
74-76: ⚡ Quick winNested
withContext(Dispatchers.Main)is unnecessary.Compose state updates are thread-safe and can be performed from any dispatcher. The
thumbnail = itassignment on line 75 can execute directly on the IO dispatcher without switching back to Main.♻️ Simplify by removing nested dispatcher switch
extractedBitmap?.let { ThumbnailCache.removeByPrefix(project.id) ThumbnailCache.put("${project.id}_${project.updated}", it) - withContext(Dispatchers.Main) { - thumbnail = it - } + thumbnail = it }🤖 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/projects/ProjectCard.kt` around lines 74 - 76, The nested withContext(Dispatchers.Main) block around the thumbnail assignment is unnecessary; remove the inner withContext(Dispatchers.Main) and assign thumbnail = it directly in the coroutine running on IO (i.e., inside the existing withContext(Dispatchers.IO) scope) so the Compose state update happens without an extra dispatcher switch—locate the thumbnail assignment in ProjectCard.kt where withContext(Dispatchers.IO) invokes an inner withContext(Dispatchers.Main) and delete the inner withContext call, leaving the thumbnail = it assignment in place.
106-112: 💤 Low valueConsider safer null handling over double-bang operator.
Line 108 uses
thumbnail!!.asImageBitmap()after a null check. While safe here, Kotlin's safe-call operator is more idiomatic and defensive against refactoring errors.♻️ Use safe-call operator
- if (thumbnail != null) { - Image( - bitmap = thumbnail!!.asImageBitmap(), - contentDescription = null, - contentScale = ContentScale.Crop, - modifier = Modifier.fillMaxSize(), - ) - } else { + thumbnail?.let { + Image( + bitmap = it.asImageBitmap(), + contentDescription = null, + contentScale = ContentScale.Crop, + modifier = Modifier.fillMaxSize(), + ) + } ?: run { Box(🤖 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/projects/ProjectCard.kt` around lines 106 - 112, The code uses a non-null assertion thumbnail!! inside the Image call in ProjectCard.kt; replace this with a safe-call to avoid NPE risks — either pass thumbnail?.asImageBitmap() or, better, scope it with thumbnail?.asImageBitmap()?.let { bitmap -> Image(bitmap = bitmap, contentDescription = null, contentScale = ContentScale.Crop, modifier = Modifier.fillMaxSize()) } so the Image composable only receives a non-null bitmap; update the Image invocation accordingly (referencing thumbnail and asImageBitmap()).
🤖 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.
Outside diff comments:
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/SyncedLyricsSelector.kt`:
- Around line 231-234: The direct call to onSelectionChanged(selected) inside
the SyncedLyricsSelector composable executes during composition and must be
moved into a side-effect; remove the inline invocation and add a LaunchedEffect
keyed on selected (e.g., LaunchedEffect(selected)) near the other effects in
SyncedLyricsSelector so the callback runs only when selection actually changes;
ensure you delete the original call (the line invoking
onSelectionChanged(selected)) so the only notifier is the new LaunchedEffect.
---
Nitpick comments:
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/LyricsSelectionModels.kt`:
- Around line 162-170: The expression computing totalSecs in function
formatDuration unnecessarily wraps the division in parentheses; update the
totalSecs assignment in formatDuration (using the parameters frames and fps) to
remove the redundant parentheses so it reads totalSecs = frames / fps, leaving
the rest of the function unchanged.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectCard.kt`:
- Around line 74-76: The nested withContext(Dispatchers.Main) block around the
thumbnail assignment is unnecessary; remove the inner
withContext(Dispatchers.Main) and assign thumbnail = it directly in the
coroutine running on IO (i.e., inside the existing withContext(Dispatchers.IO)
scope) so the Compose state update happens without an extra dispatcher
switch—locate the thumbnail assignment in ProjectCard.kt where
withContext(Dispatchers.IO) invokes an inner withContext(Dispatchers.Main) and
delete the inner withContext call, leaving the thumbnail = it assignment in
place.
- Around line 106-112: The code uses a non-null assertion thumbnail!! inside the
Image call in ProjectCard.kt; replace this with a safe-call to avoid NPE risks —
either pass thumbnail?.asImageBitmap() or, better, scope it with
thumbnail?.asImageBitmap()?.let { bitmap -> Image(bitmap = bitmap,
contentDescription = null, contentScale = ContentScale.Crop, modifier =
Modifier.fillMaxSize()) } so the Image composable only receives a non-null
bitmap; update the Image invocation accordingly (referencing thumbnail and
asImageBitmap()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5742f66a-78cb-4c23-b548-cca98f9ae062
📒 Files selected for processing (26)
modules/lyrics-maker/src/androidTest/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsScreenTest.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectsScreenCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/common/GradientText.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/common/GradientTextPreview.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/details/ProjectDetailsScreen.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/DragHandle.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/LyricRow.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/LyricsSelectionModels.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/StartDragHandle.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/SyncedLyricsSelector.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/navigation/AppNavHost.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/navigation/Screen.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/CreateNewProjectCard.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/DeleteConfirmationDialog.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/MotionProjectUpdatedLabel.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectCard.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectThumbnailExtractor.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectsRoute.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectsScreen.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ThumbnailCache.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/search/SearchScreen.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/templates/LyricsTemplateSelector.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/templates/TemplatePreviewItem.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/ProjectsViewModel.ktmodules/lyrics-maker/src/test/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ScreenTest.kt
💤 Files with no reviewable changes (1)
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectsScreenCompose.kt
✅ Files skipped from review due to trivial changes (3)
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/viewmodel/ProjectsViewModel.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ThumbnailCache.kt
🚧 Files skipped from review as they are similar to previous changes (17)
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/common/GradientTextPreview.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/MotionProjectUpdatedLabel.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/navigation/Screen.kt
- modules/lyrics-maker/src/androidTest/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsScreenTest.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/CreateNewProjectCard.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/templates/TemplatePreviewItem.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectsRoute.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/details/ProjectDetailsScreen.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/LyricRow.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectThumbnailExtractor.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/DragHandle.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/templates/LyricsTemplateSelector.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/ProjectsScreen.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/navigation/AppNavHost.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/common/GradientText.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/lyrics/StartDragHandle.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/projects/DeleteConfirmationDialog.kt
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.
Motivation
navigationpackage and a dedicatedScreentype to simplify route usage across the module.Description
presentation.compose:common,navigation,projects,lyrics,search,templates, anddetails, moving composables likeGradientText,AppNavHost/Screen,ProjectsScreen/ProjectCard/CreateNewProjectCard,SyncedLyricsSelectorand its handles/rows,SearchScreen,LyricsTemplateSelectorandTemplatePreviewItem, andProjectDetailsScreeninto discrete files.ProjectThumbnailExtractor.ktandMotionProjectUpdatedLabel.ktin theprojectspackage and movedThumbnailCacheintoprojectsas well.internalwhere appropriate) so each Kotlin file typically contains a single top-level@Composableor a tightly-scoped helper.SearchActivitynow importsnavigation.AppNavHost,ProjectsViewModelimportsprojects.ThumbnailCache,ScreenTestandProjectDetailsScreenTestadjusted imports).Testing
git diff --checkto ensure no whitespace/obvious diff errors and it completed successfully.@Composableand that check passed../gradlew :modules:lyrics-maker:compileDebugKotlinbut it could not complete in this environment because the Android SDK location is not configured (ANDROID_HOME/local.propertiesmissing)../gradlew ktlintCheckbut dependency resolution failed in this environment (ktlint artifacts returned HTTP 403), so lint checks could not be completed here.Codex Task
Summary by CodeRabbit
New Features
Refactor