templates: add motion template system for dynamic SDUI generation#61
Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 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 Kotlin template DSL and JSON templates (with {{REPLICATE}}), TemplateSerialization, SDUI view/effect/transition registration and parsers, many MotionEffect/Transition implementations, a Compose MotionVideoPlayer preview, image util, lyrics templates and selection UI, web SDUI preview updates, tests, docs, and build/module wiring. ChangesMotion Template & SDUI
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
motion-lib | d9df255 | Commit Preview URL Branch Preview URL |
May 17 2026, 06:37 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 introduces a motion template system for dynamic SDUI generation with JSON placeholder replacement functionality. The implementation is generally well-structured with good documentation and test coverage.
Critical Issues Requiring Fix:
- Crash Risk: Missing null check for
listTemplateinMotionTemplateApplier.kt(line 47) can cause NullPointerException - Logic Error: Silent exception swallowing during object-to-map conversion (lines 56-61) may lead to incorrect template processing
Additional Finding:
- Dead code in test file that should be removed for clarity
Please address the critical issues before merging to ensure the template system handles edge cases safely.
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
+56
to
+61
| try { | ||
| val json = com.google.gson.Gson().toJsonTree(item).asJsonObject | ||
| json.entrySet().associate { it.key to it.value } | ||
| } catch (e: Exception) { | ||
| emptyMap<String, Any>() | ||
| } |
Contributor
There was a problem hiding this comment.
🛑 Logic Error: Silently catching all exceptions and returning empty map can lead to incorrect template processing. Items that fail conversion will be treated as having no data, causing unexpected placeholder replacements with empty strings or original placeholder text.
| // Handle list replication | ||
| val replicationObj = value.asJsonObject | ||
| val dataKey = replicationObj.get(REPLICATE_KEY).asString | ||
| val listTemplate = replicationObj.get(TEMPLATE_KEY) |
Contributor
There was a problem hiding this comment.
🛑 Crash Risk: Missing null safety check for listTemplate can cause NullPointerException. If a replication object lacks the "template" key, get(TEMPLATE_KEY) returns null and applyToElement will crash on line 66.
Suggested change
| val listTemplate = replicationObj.get(TEMPLATE_KEY) | |
| val listTemplate = replicationObj.get(TEMPLATE_KEY) ?: return@forEach |
Comment on lines
+21
to
+24
| val data = mapOf( | ||
| "text_content" to "Hello World", | ||
| "color" to "Red" // This won't match color_code exactly unless we handle partial matches or the key is exact | ||
| ) |
Contributor
There was a problem hiding this comment.
Remove unused variable data (lines 21-24). The test uses exactData instead, making this dead code that could confuse future maintainers.
Suggested change
| val data = mapOf( | |
| "text_content" to "Hello World", | |
| "color" to "Red" // This won't match color_code exactly unless we handle partial matches or the key is exact | |
| ) | |
| val data = mapOf( | |
| "text_content" to "Hello World", | |
| "color_code" to "#FF0000" | |
| ) |
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
sdui | d9df255 | Commit Preview URL Branch Preview URL |
May 17 2026, 06:38 PM |
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
lyrics | d9df255 | Commit Preview URL Branch Preview URL |
May 17 2026, 06:38 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 introduces a new template system for the motionlib project, allowing MotionView structures to be defined via JSON templates with dynamic placeholders and list replication. Feedback focused on improving performance and robustness, specifically by utilizing Regex for single-pass placeholder replacement, sharing a Gson instance to avoid redundant allocations, and implementing null safety for replication keys. Additionally, the reviewer identified a bug where JsonPrimitive strings were being incorrectly quoted and suggested simplifying the data merging logic for better readability.
Comment on lines
+13
to
+14
| private const val PLACEHOLDER_START = "{{" | ||
| private const val PLACEHOLDER_END = "}}" |
There was a problem hiding this comment.
Instead of using simple string constants for placeholders, it is more efficient to use a Regex for single-pass replacement. Additionally, instantiating Gson repeatedly in a loop is a performance bottleneck; a shared instance should be used.
Suggested change
| private const val PLACEHOLDER_START = "{{" | |
| private const val PLACEHOLDER_END = "}}" | |
| private val gson = com.google.gson.Gson() | |
| private val placeholderRegex = Regex("\\{\\{(.+?)\\}\\}") |
Comment on lines
+46
to
+47
| val dataKey = replicationObj.get(REPLICATE_KEY).asString | ||
| val listTemplate = replicationObj.get(TEMPLATE_KEY) |
There was a problem hiding this comment.
The current implementation lacks null safety for the replication keys. If {{REPLICATE}} or template keys are missing in the JSON, the applier will crash with a NullPointerException or IllegalStateException. Using safe calls and early returns ensures robustness.
Suggested change
| val dataKey = replicationObj.get(REPLICATE_KEY).asString | |
| val listTemplate = replicationObj.get(TEMPLATE_KEY) | |
| val dataKey = replicationObj.get(REPLICATE_KEY)?.asString ?: return@forEach | |
| val listTemplate = replicationObj.get(TEMPLATE_KEY) ?: return@forEach |
| // Try to convert object to map using reflection or keep as is if primitive | ||
| // For simplicity in this example, we assume Map or we can use Gson to convert object to Map | ||
| try { | ||
| val json = com.google.gson.Gson().toJsonTree(item).asJsonObject |
There was a problem hiding this comment.
Use the shared gson instance defined at the object level to avoid unnecessary object allocations in the replication loop.
Suggested change
| val json = com.google.gson.Gson().toJsonTree(item).asJsonObject | |
| val json = gson.toJsonTree(item).asJsonObject |
Comment on lines
+89
to
+96
| private fun replacePlaceholders(input: String, data: Map<String, Any>): String { | ||
| var result = input | ||
| data.forEach { (key, value) -> | ||
| val placeholder = "$PLACEHOLDER_START$key$PLACEHOLDER_END" | ||
| result = result.replace(placeholder, value.toString()) | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
The current replacement logic is O(N * M) and has a bug where JsonPrimitive strings from Gson are converted to quoted strings (e.g., "value" instead of value). Using a Regex with a lookup map is more efficient and allows for proper handling of JsonPrimitive values.
private fun replacePlaceholders(input: String, data: Map<String, Any>): String {
return placeholderRegex.replace(input) { matchResult ->
val key = matchResult.groupValues[1]
val value = data[key] ?: return@replace matchResult.value
if (value is JsonPrimitive && value.isString) value.asString else value.toString()
}
}| } | ||
| } | ||
| // Merge item data with parent data so globals are available | ||
| val mergedData = data + itemMap.filterValues { it != null }.mapValues { it.value!! } |
There was a problem hiding this comment.
The filtering and mapping here is redundant because itemMap is already constructed from a JsonObject (where values are non-null JsonElement objects) or a Map. Simplifying this to a direct map addition improves readability and performance.
Suggested change
| val mergedData = data + itemMap.filterValues { it != null }.mapValues { it.value!! } | |
| val mergedData = data + itemMap |
| The system uses `Gson`'s `JsonObject` and `JsonArray` to traverse the template tree. It performs string replacement on any `JsonPrimitive` that is a string. | ||
|
|
||
| > [!NOTE] | ||
| > All data values are converted to strings using `.toString()` before replacement. Ensure your SDUI factories can handle the resulting string types (e.g., parsing "150" back to an Int if needed, though most SDUI parsers in this project handle stringified numbers well). |
There was a problem hiding this comment.
Update the note to reflect that JsonPrimitive strings are handled specially to avoid extra quotes, ensuring the SDUI JSON remains valid.
Suggested change
| > All data values are converted to strings using `.toString()` before replacement. Ensure your SDUI factories can handle the resulting string types (e.g., parsing "150" back to an Int if needed, though most SDUI parsers in this project handle stringified numbers well). | |
| > All data values are converted to strings before replacement. For Gson JsonPrimitive strings, the raw string value is used to avoid extra quotes. Ensure your SDUI factories can handle the resulting string types (e.g., parsing "150" back to an Int if needed, though most SDUI parsers in this project handle stringified numbers well). |
* templates: introduce `MotionTemplate` and `MotionTemplateApplier` for JSON placeholder replacement
* templates: add support for dynamic list generation using the `{{REPLICATE}}` marker
* templates: implement `MotionTemplateViewGenerator` to bridge templates with `MotionView` creation
* templates: add unit tests for placeholder logic and comprehensive module documentation
tejpratap46
force-pushed
the
feat/implement-templates
branch
from
6df55e4 to
0a5b518
Compare
![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: 2
♻️ Duplicate comments (3)
modules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/MotionTemplateApplierTest.kt (1)
21-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused variable.
The
datavariable is defined but never used. The test usesexactDatainstead, making this dead code.🧹 Proposed fix
val template = MotionTemplate(JsonParser.parseString(rawJson).asJsonObject) - val data = mapOf( - "text_content" to "Hello World", - "color" to "Red" // This won't match color_code exactly unless we handle partial matches or the key is exact - ) - // Let's use exact keys for the test val exactData = mapOf(🤖 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/test/java/com/tejpratapsingh/motionlib/templates/MotionTemplateApplierTest.kt` around lines 21 - 24, The test defines an unused local variable named data in MotionTemplateApplierTest (the map with "text_content" and "color") but the test actually uses exactData; remove the dead declaration of data to clean up the test, or if you intended to test partial key/value behavior, replace usages of exactData with data or adjust the assertions to use the data map instead; locate the unused variable named data and either delete it or swap it into the test to match intent.modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/MotionTemplateApplier.kt (2)
47-47:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winNull safety: Add null check for template field.
The
templatekey may be missing or null, which will cause aNullPointerExceptionon line 66 whenapplyToElementis called with a nulllistTemplate.🛡️ Proposed fix
- val listTemplate = replicationObj.get(TEMPLATE_KEY) + val listTemplate = replicationObj.get(TEMPLATE_KEY) ?: return@forEach🤖 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/MotionTemplateApplier.kt` at line 47, The code retrieves val listTemplate = replicationObj.get(TEMPLATE_KEY) but doesn't guard against it being null before calling applyToElement, causing an NPE; update the caller (in MotionTemplateApplier.kt where listTemplate is used) to check if listTemplate == null and either skip calling applyToElement (early return or continue) or provide a safe default/template, and/or log a warning; alternatively make applyToElement accept a nullable parameter and handle null inside it, but ensure the null-check references the exact variable name listTemplate and the method applyToElement to prevent the NPE.
56-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSilent failure: Don't hide conversion errors.
Catching all exceptions and returning
emptyMap()silently discards items that fail Gson conversion. This causes placeholders in the replicated template to remain unreplaced or use fallback values, leading to incorrect output without any warning.📋 Proposed fix
try { val json = com.google.gson.Gson().toJsonTree(item).asJsonObject json.entrySet().associate { it.key to it.value } } catch (e: Exception) { - emptyMap<String, Any>() + throw IllegalArgumentException("Failed to convert list item to map for replication: $item", e) }🤖 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/MotionTemplateApplier.kt` around lines 56 - 61, The current code in MotionTemplateApplier.kt swallows all exceptions when converting an item via com.google.gson.Gson().toJsonTree(item).asJsonObject and returns emptyMap(), hiding conversion errors; change this to not silently fail by removing the broad silent catch or at minimum replace it with a specific catch that logs the exception (including the item identity/contents and e.stackTrace) using the module's logger and then rethrow or propagate the error so the caller can handle it instead of returning emptyMap<String, Any>(); ensure the code references the same conversion expression and the previous emptyMap<String, Any>() return so reviewers can locate and update the handling.
🤖 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/templates/src/main/java/com/tejpratapsingh/motionlib/templates/MotionTemplateApplier.kt`:
- Line 46: The line using replicationObj.get(REPLICATE_KEY).asString can
NPE/IllegalState if the key exists with a null value; update the code that
assigns dataKey so you first check replicationObj.isNull(REPLICATE_KEY) (or
replicationObj.has(...) && !replicationObj.isNull(...)) and only call .asString
when non-null, otherwise handle the null case (e.g., skip replication logic, use
a default, or throw a clear exception); change the dataKey assignment (the
variable created from replicationObj.get(REPLICATE_KEY).asString) accordingly
and ensure downstream code that uses dataKey accounts for its possible
nullability.
- Line 58: The call that assumes toJsonTree(item) returns a JsonObject is
unsafe; update the code around toJsonTree(item) in MotionTemplateApplier (where
you call json.entrySet().associate { it.key to it.value }) to first check
json.isJsonObject and handle other cases (isJsonArray / isJsonPrimitive)
appropriately: if it's a JsonObject use
json.asJsonObject.entrySet().associate(...), if it's a JsonPrimitive or
JsonArray convert it to a sensible Kotlin value or wrap it (e.g., use the
JsonElement itself or toString()) so you don't call .asJsonObject on
non-objects; ensure the logic references toJsonTree and the variable named json
used in that expression.
---
Duplicate comments:
In
`@modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/MotionTemplateApplier.kt`:
- Line 47: The code retrieves val listTemplate =
replicationObj.get(TEMPLATE_KEY) but doesn't guard against it being null before
calling applyToElement, causing an NPE; update the caller (in
MotionTemplateApplier.kt where listTemplate is used) to check if listTemplate ==
null and either skip calling applyToElement (early return or continue) or
provide a safe default/template, and/or log a warning; alternatively make
applyToElement accept a nullable parameter and handle null inside it, but ensure
the null-check references the exact variable name listTemplate and the method
applyToElement to prevent the NPE.
- Around line 56-61: The current code in MotionTemplateApplier.kt swallows all
exceptions when converting an item via
com.google.gson.Gson().toJsonTree(item).asJsonObject and returns emptyMap(),
hiding conversion errors; change this to not silently fail by removing the broad
silent catch or at minimum replace it with a specific catch that logs the
exception (including the item identity/contents and e.stackTrace) using the
module's logger and then rethrow or propagate the error so the caller can handle
it instead of returning emptyMap<String, Any>(); ensure the code references the
same conversion expression and the previous emptyMap<String, Any>() return so
reviewers can locate and update the handling.
In
`@modules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/MotionTemplateApplierTest.kt`:
- Around line 21-24: The test defines an unused local variable named data in
MotionTemplateApplierTest (the map with "text_content" and "color") but the test
actually uses exactData; remove the dead declaration of data to clean up the
test, or if you intended to test partial key/value behavior, replace usages of
exactData with data or adjust the assertions to use the data map instead; locate
the unused variable named data and either delete it or swap it into the test to
match intent.
🪄 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: d92bb5d4-1ba9-4606-b43f-8f527f174e8b
📒 Files selected for processing (6)
modules/templates/README.mdmodules/templates/build.gradlemodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/MotionTemplate.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/MotionTemplateApplier.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/MotionTemplateViewGenerator.ktmodules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/MotionTemplateApplierTest.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (10)
modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/dsl/MotionTemplateDSL.kt (1)
48-49: ⚡ Quick winReturn immutable snapshots from builder
build()methods.On Line 48 and Line 111, both methods expose mutable backing lists. Returning copies avoids accidental post-build mutation leaks.
Suggested fix
- fun build(): List<MotionEffect> = effects + fun build(): List<MotionEffect> = effects.toList() ... - fun build(): List<TemplateParameter<*>> = parameters + fun build(): List<TemplateParameter<*>> = parameters.toList()Also applies to: 111-112
🤖 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/dsl/MotionTemplateDSL.kt` around lines 48 - 49, The build() methods currently return the mutable backing list (effects) which allows callers to mutate internal state; update both build() implementations (the functions named build() that currently return effects) to return an immutable snapshot by returning a copy of the list (e.g., use effects.toList() or an equivalent copy operation) so callers receive an immutable snapshot rather than the original mutable backing list.modules/templates/src/androidTest/java/com/tejpratapsingh/motionlib/templates/MotionTemplateInstrumentationTest.kt (2)
98-99: ⚡ Quick winExtract magic numbers to named constants.
The hard-coded values
4and1make the assertions fragile and harder to understand. If the template structure changes (e.g., adding another view type), these would need manual updates.♻️ Proposed refactor for clarity
+ // Expected counts: 1 video + 2 lyrics + 1 title = 4 views, 1 audio + val expectedViewCount = 4 + val expectedAudioCount = 1 + // Verify that views and audio were actually added to the producer - assertEquals(4, producer.motionComposerView.childCount) // 1 video + 2 lyrics + 1 title - assertEquals(1, producer.motionAudio.size) + assertEquals(expectedViewCount, producer.motionComposerView.childCount) + assertEquals(expectedAudioCount, producer.motionAudio.size)🤖 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/MotionTemplateInstrumentationTest.kt` around lines 98 - 99, Replace the magic numbers in the assertions with named constants or computed values so intent is clear and the test is resilient: introduce constants like EXPECTED_CHILD_COUNT (or compose from VIDEO_COUNT, LYRIC_COUNT, TITLE_COUNT) and EXPECTED_AUDIO_COUNT and use those in the assertions against producer.motionComposerView.childCount and producer.motionAudio.size; alternatively compute expected counts from the template definition/helpers and assert equality to those named/derived values to avoid hard-coded literals.
73-73: 💤 Low valueDocument that test uses non-existent audio file.
The test references
test_audio.mp3in the cache directory, which likely doesn't exist. While this is acceptable for testing DSL registration (the test only verifies that an audio entry was added to the producer's list), consider adding a comment to clarify this is intentional and explain that actual playback would require a real file.📝 Proposed documentation improvement
// Test audio function + // Note: File doesn't need to exist for this test, we're only verifying DSL registration audio(File(appContext.cacheDir, "test_audio.mp3"), startFrame = 0, endFrame = 500)🤖 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/MotionTemplateInstrumentationTest.kt` at line 73, Add a brief inline comment in MotionTemplateInstrumentationTest.kt next to the audio(File(appContext.cacheDir, "test_audio.mp3"), startFrame = 0, endFrame = 500) call to state that the referenced "test_audio.mp3" is intentionally not present in the cache (the test only validates DSL registration/that an audio entry is added to the producer list and does not perform playback), and note that real playback would require a real file on disk; this clarifies intent for future readers and maintainers.modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/serialization/TemplateSerialization.kt (2)
119-129: ⚡ Quick winUse StringBuilder instead of StringBuffer for better performance.
StringBufferis synchronized, which adds unnecessary overhead in this single-threaded context.StringBuilderis the preferred choice for string concatenation.⚡ Proposed refactor to use StringBuilder
} else if (matcher.find()) { // Partial match, string interpolation - val sb = StringBuffer() + val sb = StringBuilder() matcher.reset()🤖 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/serialization/TemplateSerialization.kt` around lines 119 - 129, The code in the partial-match branch of TemplateSerialization (the matcher loop that currently constructs sb using StringBuffer) should use StringBuilder instead of the synchronized StringBuffer to avoid unnecessary overhead; replace the instantiation of StringBuffer with StringBuilder and update uses of matcher.appendReplacement and matcher.appendTail to work with the StringBuilder (ensure you pass a java.lang.StringBuffer-compatible wrapper if needed or convert to a String for appendReplacement/appendTail calls), keeping the same logic around paramName, value lookup from data.get and returning JsonPrimitive(sb.toString()).
72-76: ⚡ Quick winConsider throwing an error when {{REPLICATE}} is misused.
The comment acknowledges that encountering
{{REPLICATE}}in an object (not inside an array) means it's "misused", but the code silently returns the element unchanged. This could mask authoring errors in templates.
⚠️ Proposed improvement to detect misuseif (obj.has("{{REPLICATE}}")) { - // This object is a replication marker, but it should be handled by the parent array - // If it's encountered here, it means it's not inside an array or misused. - // We'll return it as is or handle it if we want to support single-item replication - element + throw IllegalArgumentException("{{REPLICATE}} marker must be used inside an array, not as a standalone object") } else {Alternatively, log a warning if strict errors are undesirable.
🤖 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/serialization/TemplateSerialization.kt` around lines 72 - 76, The code in TemplateSerialization.kt currently silently returns the JSON element when encountering obj.has("{{REPLICATE}}") outside of an array (the block that simply yields element), which masks template authoring errors; update the handling in that branch (the obj.has("{{REPLICATE}}") check inside the deserialization logic) to throw a clear, descriptive exception (or at minimum log a warning) indicating "{{REPLICATE}} must be used inside an array" so misuse is surfaced at parse time rather than swallowed by returning element; ensure the thrown exception type and message are consistent with other deserialization errors in this class so callers can handle it.modules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/TemplateSerializationTest.kt (1)
16-107: ⚡ Quick winConsider adding edge case tests for robustness.
The current tests cover happy paths well, but consider adding tests for edge cases:
- Missing placeholders (data not provided for a
{{param}})- Null values in
TemplateData- Malformed placeholders (e.g.,
{{unclosed,{{}})- Empty list for
{{REPLICATE}}- Type mismatches (e.g., expecting integer but getting string)
These additions would increase confidence in the error handling and robustness of the serialization logic.
🤖 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/test/java/com/tejpratapsingh/motionlib/templates/TemplateSerializationTest.kt` around lines 16 - 107, Add unit tests in TemplateSerializationTest.kt exercising TemplateSerialization.applyData and templateFromJson for edge cases: (1) missing placeholders - applyData should leave unreplaced tokens or throw/return informative error depending on current behavior, (2) null values in TemplateData entries, (3) malformed placeholders like "{{unclosed" and "{{}}" to assert graceful handling, (4) empty list for a "{{REPLICATE}}" source to ensure no elements are inserted and structure remains valid, and (5) type mismatches (e.g., string where integer expected) to assert either coercion or a clear failure; name tests clearly (e.g., testMissingPlaceholder, testNullValues, testMalformedPlaceholder, testEmptyReplicateList, testTypeMismatch) and assert the expected outputs/behaviors from TemplateSerialization.applyData and TemplateSerialization.templateFromJson.modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/json/JsonMotionTemplate.kt (2)
26-35: ⚡ Quick winConsider logging or documenting when non-ViewGroup views are silently ignored.
The code silently ignores any views where
toMotionViewreturns a non-ViewGroupinstance. This could make debugging difficult when expected views don't appear in the output.📝 Proposed improvement to add debug logging
val motionView = viewJson.asJsonObject.toMotionView(scope.context) if (motionView is ViewGroup) { scope.addView(motionView) + } else { + android.util.Log.w("JsonMotionTemplate", "Skipping non-ViewGroup view: ${motionView.javaClass.simpleName}") }Apply the same pattern at lines 32-35 for the single-view case.
🤖 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/json/JsonMotionTemplate.kt` around lines 26 - 35, The code silently ignores cases where toMotionView(...) returns a non-ViewGroup (both in the loop branch using viewJson.asJsonObject.toMotionView(scope.context) and the single-view branch using appliedJson.toMotionView(scope.context)); add a debug log or comment in both places to record when motionView is not a ViewGroup (before/after the if (motionView is ViewGroup) check) so callers can see when expected views are skipped; reference the motionView variable, the toMotionView(...) calls and the scope.addView(...) call when adding the log or documentation.
17-37: ⚡ Quick winAdd error handling for JSON-to-view conversion.
The
toMotionViewcalls (lines 26, 32) may throw exceptions if the JSON structure is invalid or unsupported. Consider wrapping these calls in try-catch blocks to provide more helpful error messages.🛡️ Proposed improvement to add error handling
override fun buildContent(scope: ContentScope) { + try { // 1. Apply data to content JSON val appliedJson = TemplateSerialization.applyData(rawContent, scope.data).asJsonObject // 2. Content can be a single view or a container with children // For simplicity, let's assume it's a list of views under "views" key or a single view if (appliedJson.has("views")) { val viewsArray = appliedJson.getAsJsonArray("views") viewsArray.forEach { viewJson -> val motionView = viewJson.asJsonObject.toMotionView(scope.context) if (motionView is ViewGroup) { scope.addView(motionView) } } } else { val motionView = appliedJson.toMotionView(scope.context) if (motionView is ViewGroup) { scope.addView(motionView) } } + } catch (e: Exception) { + throw IllegalStateException("Failed to build content for template '$name': ${e.message}", e) + } }🤖 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/json/JsonMotionTemplate.kt` around lines 17 - 37, The buildContent method may throw when converting JSON to views; wrap the two calls to toMotionView (the one inside the viewsArray loop and the one for the single-view branch) in try-catch blocks so malformed/unsupported JSON doesn't crash rendering: catch the exception, log a clear error that includes context (e.g., the failing appliedJson or viewJson and the exception) and then skip adding that view (i.e., don't call scope.addView on failure) so the method continues processing other views; ensure you use existing logging conventions available in this class or scope to report the error rather than swallowing it silently.modules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/MotionTemplateTest.kt (2)
68-114: 💤 Low valueConsider the same pattern for the second test.
Similar to the first test, the
testHardExampleFromReadmehas assertions in the content block (lines 70, 74, 78, 83) and createsTemplateData(lines 99-106), but never executesbuildContent. The test only verifies template structure, not runtime behavior.This is acknowledged in comments at lines 108-111, but consider either removing the content block assertions or adding a note that these serve as documentation/examples rather than executable tests.
🤖 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/test/java/com/tejpratapsingh/motionlib/templates/MotionTemplateTest.kt` around lines 68 - 114, The second test (testHardExampleFromReadme) contains runtime assertions inside the content { ... } block and constructs TemplateData (symbols: content block, TemplateData, lyricsVideoTemplate, audio(...) and producer.addAudio) but never calls buildContent, so those assertions are not executed and risk misleading readers or causing NPEs if run; fix by either removing/moving the assertions out of the content block (leave only metadata checks like lyricsVideoTemplate.parameters.size) or explicitly execute the template with buildContent while providing a safe stub/mock for MotionVideoProducer/Context (or mark the test `@Ignore/`@Disabled) so audio(...) and producer.addAudio cannot NPE — choose one approach and update testHardExampleFromReadme accordingly.
20-49: ⚡ Quick winTest assertions inside content block are never executed.
The
assertEqualscalls at lines 24-25 are inside thecontentblock, which is only executed whenbuildContentis called. This test creates aTemplateDataobject (lines 36-42) but never callsbuildContent, so the assertions checking data extraction never run. The test only validates template metadata (name, parameters), not the actual content logic.If the goal is to test metadata only, remove the assertions from the content block. If you want to test content execution, you need to create a
ContentScopeand callbuildContent.💡 Option 1: Remove unused assertions from content block
content { val title = data.getString("title") val duration = data.getInt("duration") - - assertEquals("Hello World", title) - assertEquals(200, duration) }💡 Option 2: Add mock ContentScope to verify content execution
// After template creation, add: val mockProducer = mock(MotionVideoProducer::class.java) val mockContext = mock(Context::class.java) val scope = ContentScope(mockContext, mockProducer, data) // This would execute the content block including assertions template.buildContent(scope)Note: This requires mocking framework dependencies.
🤖 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/test/java/com/tejpratapsingh/motionlib/templates/MotionTemplateTest.kt` around lines 20 - 49, The assertions inside the content block never run because the DSL block is only executed when buildContent is invoked; either remove those assertions from the content block if you only intend to test metadata, or invoke the content execution by creating a ContentScope with a TemplateData and calling template.buildContent(scope). To run the content path, construct a ContentScope (or equivalent real objects) with a mock or real Context and MotionVideoProducer, pass the TemplateData map you already create, then call template.buildContent(scope) so the content { ... } assertions execute.
🤖 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/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/MotionVideoProducer.kt`:
- Around line 53-70: The horizontal gravity checks in MotionVideoProducer.kt
currently only test LEFT/RIGHT so START/END settings fall through to centering;
update the x assignment logic to also check for android.view.Gravity.START
(treat as left) and android.view.Gravity.END (treat as right) alongside
LEFT/RIGHT (e.g. change the second/third branches to "layoutInfo.gravity and
Gravity.START == Gravity.START" and "layoutInfo.gravity and Gravity.END ==
Gravity.END" or combine checks with OR), keeping the existing
centerHorizontallyTo(), leftTo(), and rightTo() branches unchanged so START/END
are handled the same as LEFT/RIGHT.
In `@modules/templates/build.gradle`:
- Line 52: Change the Gson dependency from an implementation to an API
dependency so consumers can see Gson types used in your public surface: replace
the dependency declaration using libs.gson under implementation with an api
configuration; this is required because JsonMotionTemplate exposes the public
property rawContent: JsonObject (a Gson type) and consumers need access to the
Gson classes to compile against that API.
In
`@modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/json/JsonMotionTemplate.kt`:
- Line 19: The code calls TemplateSerialization.applyData(rawContent,
scope.data).asJsonObject without validating the JsonElement type; update
JsonMotionTemplate.kt to first capture the JsonElement (from
TemplateSerialization.applyData), check element.isJsonObject (or safely cast),
and only then call asJsonObject; if the result is not a JsonObject, handle it
explicitly (throw a clear IllegalArgumentException or return a meaningful
error/log) so we avoid an unchecked IllegalStateException at runtime.
In
`@modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/model/TemplateData.kt`:
- Around line 8-10: The typed getters getInt(name: String) and getFloat(name:
String) currently perform strict casts and will return null for other Number
subtypes; change them to retrieve the raw value via get(name), then if the value
is a Number convert via toInt()/toFloat(), else if it's a numeric String attempt
parsing, otherwise return null; update getBoolean(name: String) similarly to
accept Boolean or parse "true"/"false" strings if present so dynamic template
data (e.g., Long, Double, String) is coerced correctly.
In
`@modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/serialization/TemplateSerialization.kt`:
- Around line 123-126: The replacement string passed to
matcher.appendReplacement is not escaped, so special characters like '$' and '\'
in template values (e.g., currency) will be misinterpreted; update the code in
TemplateSerialization.kt where the Matcher is used (the matcher variable and its
appendReplacement call) to wrap the computed value with a safe-escape function
(use Matcher.quoteReplacement or equivalent) before calling
matcher.appendReplacement(sb, ...), ensuring null/empty handling remains the
same.
- Around line 89-98: The code assumes data.get<List<Map<String, Any>>>(listKey)
returns a List<Map<String, Any>> and will ClassCastException if it's not; update
the replicate handling (inside the block checking item.isJsonObject and
"{{REPLICATE}}") to first retrieve listData as Any?, validate it is a List<*>,
then for each element ensure the element is a Map<*,*> and safely coerce/convert
keys to String to create TemplateData (or skip non-map elements), and only then
call applyData(template, itemTemplateData) to add to newArray; reference the
symbols listKey, listData, TemplateData, applyData and the replicateObj/template
handling so you add a type guard and graceful skip/logging for invalid types
instead of an unchecked cast.
- Line 10: The PLACEHOLDER_PATTERN currently uses .+? which can capture closing
braces and allow malformed nested placeholders; update the Pattern used by
PLACEHOLDER_PATTERN in TemplateSerialization.kt to disallow '}' inside the
capture group by replacing the .+? with a negated-character class (i.e. use
[^}]+ instead of .+?) so placeholder names cannot contain closing braces, then
run tests to ensure behavior is unchanged for valid placeholders.
- Line 53: The call to ParameterType.valueOf(json.get("type").asString) can
throw IllegalArgumentException for invalid strings; update the parsing in
TemplateSerialization (the block that assigns 'type') to catch
IllegalArgumentException (or validate the string against
ParameterType.values()/names()) and handle it explicitly—either set a safe
default, return a clear parsing error/exception with the invalid value, or log a
descriptive message; ensure you reference the json.get("type").asString input
and replace the direct ParameterType.valueOf call with the guarded logic in the
same method so malformed JSON doesn't cause an uncaught exception.
---
Nitpick comments:
In
`@modules/templates/src/androidTest/java/com/tejpratapsingh/motionlib/templates/MotionTemplateInstrumentationTest.kt`:
- Around line 98-99: Replace the magic numbers in the assertions with named
constants or computed values so intent is clear and the test is resilient:
introduce constants like EXPECTED_CHILD_COUNT (or compose from VIDEO_COUNT,
LYRIC_COUNT, TITLE_COUNT) and EXPECTED_AUDIO_COUNT and use those in the
assertions against producer.motionComposerView.childCount and
producer.motionAudio.size; alternatively compute expected counts from the
template definition/helpers and assert equality to those named/derived values to
avoid hard-coded literals.
- Line 73: Add a brief inline comment in MotionTemplateInstrumentationTest.kt
next to the audio(File(appContext.cacheDir, "test_audio.mp3"), startFrame = 0,
endFrame = 500) call to state that the referenced "test_audio.mp3" is
intentionally not present in the cache (the test only validates DSL
registration/that an audio entry is added to the producer list and does not
perform playback), and note that real playback would require a real file on
disk; this clarifies intent for future readers and maintainers.
In
`@modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/dsl/MotionTemplateDSL.kt`:
- Around line 48-49: The build() methods currently return the mutable backing
list (effects) which allows callers to mutate internal state; update both
build() implementations (the functions named build() that currently return
effects) to return an immutable snapshot by returning a copy of the list (e.g.,
use effects.toList() or an equivalent copy operation) so callers receive an
immutable snapshot rather than the original mutable backing list.
In
`@modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/json/JsonMotionTemplate.kt`:
- Around line 26-35: The code silently ignores cases where toMotionView(...)
returns a non-ViewGroup (both in the loop branch using
viewJson.asJsonObject.toMotionView(scope.context) and the single-view branch
using appliedJson.toMotionView(scope.context)); add a debug log or comment in
both places to record when motionView is not a ViewGroup (before/after the if
(motionView is ViewGroup) check) so callers can see when expected views are
skipped; reference the motionView variable, the toMotionView(...) calls and the
scope.addView(...) call when adding the log or documentation.
- Around line 17-37: The buildContent method may throw when converting JSON to
views; wrap the two calls to toMotionView (the one inside the viewsArray loop
and the one for the single-view branch) in try-catch blocks so
malformed/unsupported JSON doesn't crash rendering: catch the exception, log a
clear error that includes context (e.g., the failing appliedJson or viewJson and
the exception) and then skip adding that view (i.e., don't call scope.addView on
failure) so the method continues processing other views; ensure you use existing
logging conventions available in this class or scope to report the error rather
than swallowing it silently.
In
`@modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/serialization/TemplateSerialization.kt`:
- Around line 119-129: The code in the partial-match branch of
TemplateSerialization (the matcher loop that currently constructs sb using
StringBuffer) should use StringBuilder instead of the synchronized StringBuffer
to avoid unnecessary overhead; replace the instantiation of StringBuffer with
StringBuilder and update uses of matcher.appendReplacement and
matcher.appendTail to work with the StringBuilder (ensure you pass a
java.lang.StringBuffer-compatible wrapper if needed or convert to a String for
appendReplacement/appendTail calls), keeping the same logic around paramName,
value lookup from data.get and returning JsonPrimitive(sb.toString()).
- Around line 72-76: The code in TemplateSerialization.kt currently silently
returns the JSON element when encountering obj.has("{{REPLICATE}}") outside of
an array (the block that simply yields element), which masks template authoring
errors; update the handling in that branch (the obj.has("{{REPLICATE}}") check
inside the deserialization logic) to throw a clear, descriptive exception (or at
minimum log a warning) indicating "{{REPLICATE}} must be used inside an array"
so misuse is surfaced at parse time rather than swallowed by returning element;
ensure the thrown exception type and message are consistent with other
deserialization errors in this class so callers can handle it.
In
`@modules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/MotionTemplateTest.kt`:
- Around line 68-114: The second test (testHardExampleFromReadme) contains
runtime assertions inside the content { ... } block and constructs TemplateData
(symbols: content block, TemplateData, lyricsVideoTemplate, audio(...) and
producer.addAudio) but never calls buildContent, so those assertions are not
executed and risk misleading readers or causing NPEs if run; fix by either
removing/moving the assertions out of the content block (leave only metadata
checks like lyricsVideoTemplate.parameters.size) or explicitly execute the
template with buildContent while providing a safe stub/mock for
MotionVideoProducer/Context (or mark the test `@Ignore/`@Disabled) so audio(...)
and producer.addAudio cannot NPE — choose one approach and update
testHardExampleFromReadme accordingly.
- Around line 20-49: The assertions inside the content block never run because
the DSL block is only executed when buildContent is invoked; either remove those
assertions from the content block if you only intend to test metadata, or invoke
the content execution by creating a ContentScope with a TemplateData and calling
template.buildContent(scope). To run the content path, construct a ContentScope
(or equivalent real objects) with a mock or real Context and
MotionVideoProducer, pass the TemplateData map you already create, then call
template.buildContent(scope) so the content { ... } assertions execute.
In
`@modules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/TemplateSerializationTest.kt`:
- Around line 16-107: Add unit tests in TemplateSerializationTest.kt exercising
TemplateSerialization.applyData and templateFromJson for edge cases: (1) missing
placeholders - applyData should leave unreplaced tokens or throw/return
informative error depending on current behavior, (2) null values in TemplateData
entries, (3) malformed placeholders like "{{unclosed" and "{{}}" to assert
graceful handling, (4) empty list for a "{{REPLICATE}}" source to ensure no
elements are inserted and structure remains valid, and (5) type mismatches
(e.g., string where integer expected) to assert either coercion or a clear
failure; name tests clearly (e.g., testMissingPlaceholder, testNullValues,
testMalformedPlaceholder, testEmptyReplicateList, testTypeMismatch) and assert
the expected outputs/behaviors from TemplateSerialization.applyData and
TemplateSerialization.templateFromJson.
🪄 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: d396cd93-0d73-4e54-87e6-804e603164f8
📒 Files selected for processing (18)
modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/MotionVideoProducer.ktmodules/templates/README.mdmodules/templates/build.gradlemodules/templates/src/androidTest/java/com/tejpratapsingh/motionlib/templates/MotionTemplateInstrumentationTest.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/dsl/MotionTemplateDSL.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/AudioExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/BackgroundExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/ContainerExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/ImageExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/TextExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/VideoExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/json/JsonMotionTemplate.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/model/MotionTemplate.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/model/TemplateData.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/model/TemplateParameter.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/serialization/TemplateSerialization.ktmodules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/MotionTemplateTest.ktmodules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/TemplateSerializationTest.kt
✅ Files skipped from review due to trivial changes (2)
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/model/MotionTemplate.kt
- modules/templates/README.md
Comment on lines
+53
to
+70
| x = if (layoutInfo.gravity and android.view.Gravity.CENTER_HORIZONTAL == android.view.Gravity.CENTER_HORIZONTAL) { | ||
| centerHorizontallyTo { parent.centerX() } | ||
| } else if (layoutInfo.gravity and android.view.Gravity.LEFT == android.view.Gravity.LEFT) { | ||
| leftTo { parent.left() + layoutInfo.margin.left.toXInt() } | ||
| } else if (layoutInfo.gravity and android.view.Gravity.RIGHT == android.view.Gravity.RIGHT) { | ||
| rightTo { parent.right() - layoutInfo.margin.right.toXInt() } | ||
| } else { | ||
| centerHorizontallyTo { parent.centerX() } | ||
| }, | ||
| y = if (layoutInfo.gravity and android.view.Gravity.CENTER_VERTICAL == android.view.Gravity.CENTER_VERTICAL) { | ||
| centerVerticallyTo { parent.centerY() } | ||
| } else if (layoutInfo.gravity and android.view.Gravity.TOP == android.view.Gravity.TOP) { | ||
| topTo { parent.top() + layoutInfo.margin.top.toYInt() } | ||
| } else if (layoutInfo.gravity and android.view.Gravity.BOTTOM == android.view.Gravity.BOTTOM) { | ||
| bottomTo { parent.bottom() - layoutInfo.margin.bottom.toYInt() } | ||
| } else { | ||
| centerVerticallyTo { parent.centerY() } | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether START/END gravity is currently used by template or producer callers.
# Expected: Any matches indicate the bug is immediately user-visible.
rg -nP --type=kt 'Gravity\.(START|END)'
rg -nP --type=kt 'MotionLayoutInfo\([^)]*gravity\s*='Repository: tejpratap46/AndroidVideoMotion
Length of output: 128
🏁 Script executed:
#!/bin/bash
# Search for Kotlin files and gravity usage
rg -g '*.kt' 'Gravity\.(START|END)' -n
rg -g '*.kt' 'MotionLayoutInfo' -n | head -20Repository: tejpratap46/AndroidVideoMotion
Length of output: 3589
🏁 Script executed:
# Read the MotionVideoProducer.kt file
find . -name "MotionVideoProducer.kt" -type fRepository: tejpratap46/AndroidVideoMotion
Length of output: 172
🏁 Script executed:
# Read the MotionVideoProducer.kt file around lines 53-70
cat -n ./modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/MotionVideoProducer.kt | sed -n '1,100p'Repository: tejpratap46/AndroidVideoMotion
Length of output: 4808
🏁 Script executed:
# Check if layoutDirection is used or available in the file
rg -g '*.kt' -A5 -B5 'layoutDirection' modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/MotionVideoProducer.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 56
🏁 Script executed:
# Find MotionComposerView class
find . -name "MotionComposerView.kt" -type fRepository: tejpratap46/AndroidVideoMotion
Length of output: 171
🏁 Script executed:
# Check if layoutDirection is available in Android views/any related classes
rg -g '*.kt' 'layoutDirection' modules/motionlib/ | head -20Repository: tejpratap46/AndroidVideoMotion
Length of output: 56
🏁 Script executed:
# Read MotionComposerView.kt
cat -n ./modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/MotionComposerView.kt | head -100Repository: tejpratap46/AndroidVideoMotion
Length of output: 2275
🏁 Script executed:
# Search for layoutDirection in Android framework context (it's a View property)
# Check if MotionComposerView extends ViewGroup
rg -g '*.kt' 'class MotionComposerView' -A 5Repository: tejpratap46/AndroidVideoMotion
Length of output: 869
🏁 Script executed:
# Verify Gravity import and getAbsoluteGravity availability
rg -g '*.kt' 'Gravity\.getAbsoluteGravity' --context=2Repository: tejpratap46/AndroidVideoMotion
Length of output: 56
🏁 Script executed:
# Check ContourLayout source to confirm it's a ViewGroup
rg -g '*.kt' 'ContourLayout' modules/motionlib/ | head -10Repository: tejpratap46/AndroidVideoMotion
Length of output: 867
Handle START/END gravity so relative positioning doesn't fall back to center.
Lines 55 and 57 only check LEFT/RIGHT. With Gravity.START or Gravity.END (which are set by users in MotionViewParser), horizontal placement incorrectly falls through to centering.
Suggested fix
+import android.view.Gravity
@@
- x = if (layoutInfo.gravity and android.view.Gravity.CENTER_HORIZONTAL == android.view.Gravity.CENTER_HORIZONTAL) {
+ val absoluteGravity = Gravity.getAbsoluteGravity(layoutInfo.gravity, layoutDirection)
+ x = if (absoluteGravity and Gravity.CENTER_HORIZONTAL == Gravity.CENTER_HORIZONTAL) {
centerHorizontallyTo { parent.centerX() }
- } else if (layoutInfo.gravity and android.view.Gravity.LEFT == android.view.Gravity.LEFT) {
+ } else if (absoluteGravity and Gravity.LEFT == Gravity.LEFT) {
leftTo { parent.left() + layoutInfo.margin.left.toXInt() }
- } else if (layoutInfo.gravity and android.view.Gravity.RIGHT == android.view.Gravity.RIGHT) {
+ } else if (absoluteGravity and Gravity.RIGHT == Gravity.RIGHT) {
rightTo { parent.right() - layoutInfo.margin.right.toXInt() }
} else {
centerHorizontallyTo { parent.centerX() }
},🤖 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/core/motion/MotionVideoProducer.kt`
around lines 53 - 70, The horizontal gravity checks in MotionVideoProducer.kt
currently only test LEFT/RIGHT so START/END settings fall through to centering;
update the x assignment logic to also check for android.view.Gravity.START
(treat as left) and android.view.Gravity.END (treat as right) alongside
LEFT/RIGHT (e.g. change the second/third branches to "layoutInfo.gravity and
Gravity.START == Gravity.START" and "layoutInfo.gravity and Gravity.END ==
Gravity.END" or combine checks with OR), keeping the existing
centerHorizontallyTo(), leftTo(), and rightTo() branches unchanged so START/END
are handled the same as LEFT/RIGHT.
| implementation project(path: ':modules:core') | ||
| implementation project(path: ':modules:sdui') | ||
| implementation libs.androidx.appcompat | ||
| implementation libs.gson |
Contributor
There was a problem hiding this comment.
Gson should be declared as 'api' instead of 'implementation'.
The JsonMotionTemplate class exposes rawContent: JsonObject as a public property. Since JsonObject is a Gson type, consumers of this module will need access to Gson classes to work with this API. Using implementation hides Gson from consumers, potentially causing compilation errors.
🔧 Proposed fix to expose Gson to API consumers
implementation project(path: ':modules:sdui')
implementation libs.androidx.appcompat
- implementation libs.gson
+ api libs.gson📝 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
| implementation libs.gson | |
| api libs.gson |
🤖 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/build.gradle` at line 52, Change the Gson dependency from
an implementation to an API dependency so consumers can see Gson types used in
your public surface: replace the dependency declaration using libs.gson under
implementation with an api configuration; this is required because
JsonMotionTemplate exposes the public property rawContent: JsonObject (a Gson
type) and consumers need access to the Gson classes to compile against that API.
|
|
||
| override fun buildContent(scope: ContentScope) { | ||
| // 1. Apply data to content JSON | ||
| val appliedJson = TemplateSerialization.applyData(rawContent, scope.data).asJsonObject |
Contributor
There was a problem hiding this comment.
Add type safety check before casting to JsonObject.
applyData returns JsonElement, but the code directly calls .asJsonObject without verifying the result is actually a JsonObject. If applyData returns a different type (e.g., JsonArray or JsonPrimitive), this will throw IllegalStateException at runtime.
🛡️ Proposed fix to add type validation
- val appliedJson = TemplateSerialization.applyData(rawContent, scope.data).asJsonObject
+ val appliedElement = TemplateSerialization.applyData(rawContent, scope.data)
+ if (!appliedElement.isJsonObject) {
+ throw IllegalStateException("Applied template data must be a JsonObject, got: ${appliedElement.javaClass.simpleName}")
+ }
+ val appliedJson = appliedElement.asJsonObject🤖 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/json/JsonMotionTemplate.kt`
at line 19, The code calls TemplateSerialization.applyData(rawContent,
scope.data).asJsonObject without validating the JsonElement type; update
JsonMotionTemplate.kt to first capture the JsonElement (from
TemplateSerialization.applyData), check element.isJsonObject (or safely cast),
and only then call asJsonObject; if the result is not a JsonObject, handle it
explicitly (throw a clear IllegalArgumentException or return a meaningful
error/log) so we avoid an unchecked IllegalStateException at runtime.
Comment on lines
+8
to
+10
| fun getInt(name: String): Int? = get(name) | ||
| fun getFloat(name: String): Float? = get(name) | ||
| fun getBoolean(name: String): Boolean? = get(name) |
Contributor
There was a problem hiding this comment.
Handle numeric coercion in typed getters.
On Line 8 and Line 9, strict casts only accept exact boxed types (Int/Float). Values stored as other Number subtypes will silently return null, which is easy to hit in dynamic template data flows.
Suggested fix
class TemplateData(private val values: Map<String, Any>) {
`@Suppress`("UNCHECKED_CAST")
fun <T> get(name: String): T? = values[name] as? T
fun getString(name: String): String? = get(name)
- fun getInt(name: String): Int? = get(name)
- fun getFloat(name: String): Float? = get(name)
+ fun getInt(name: String): Int? =
+ when (val value = values[name]) {
+ is Int -> value
+ is Number -> value.toInt()
+ is String -> value.toIntOrNull()
+ else -> null
+ }
+
+ fun getFloat(name: String): Float? =
+ when (val value = values[name]) {
+ is Float -> value
+ is Number -> value.toFloat()
+ is String -> value.toFloatOrNull()
+ else -> null
+ }
fun getBoolean(name: String): Boolean? = get(name)
}📝 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
| fun getInt(name: String): Int? = get(name) | |
| fun getFloat(name: String): Float? = get(name) | |
| fun getBoolean(name: String): Boolean? = get(name) | |
| fun getInt(name: String): Int? = | |
| when (val value = values[name]) { | |
| is Int -> value | |
| is Number -> value.toInt() | |
| is String -> value.toIntOrNull() | |
| else -> null | |
| } | |
| fun getFloat(name: String): Float? = | |
| when (val value = values[name]) { | |
| is Float -> value | |
| is Number -> value.toFloat() | |
| is String -> value.toFloatOrNull() | |
| else -> null | |
| } | |
| fun getBoolean(name: String): Boolean? = get(name) |
🤖 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/model/TemplateData.kt`
around lines 8 - 10, The typed getters getInt(name: String) and getFloat(name:
String) currently perform strict casts and will return null for other Number
subtypes; change them to retrieve the raw value via get(name), then if the value
is a Number convert via toInt()/toFloat(), else if it's a numeric String attempt
parsing, otherwise return null; update getBoolean(name: String) similarly to
accept Boolean or parse "true"/"false" strings if present so dynamic template
data (e.g., Long, Double, String) is coerced correctly.
|
|
||
| object TemplateSerialization { | ||
| private val GSON = GsonBuilder().setPrettyPrinting().create() | ||
| private val PLACEHOLDER_PATTERN = Pattern.compile("\\{\\{(.+?)\\}\\}") |
Contributor
There was a problem hiding this comment.
Placeholder pattern may incorrectly match nested braces.
The regex pattern \\{\\{(.+?)\\}\\} uses .+? which can match content containing } characters, potentially capturing malformed or nested placeholders like {{a{{b}} → would extract a{{b.
🔧 Proposed fix to prevent matching closing braces
- private val PLACEHOLDER_PATTERN = Pattern.compile("\\{\\{(.+?)\\}\\}")
+ private val PLACEHOLDER_PATTERN = Pattern.compile("\\{\\{([^}]+)\\}\\}")This ensures placeholder names cannot contain } characters.
📝 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
| private val PLACEHOLDER_PATTERN = Pattern.compile("\\{\\{(.+?)\\}\\}") | |
| private val PLACEHOLDER_PATTERN = Pattern.compile("\\{\\{([^}]+)\\}\\}") |
🤖 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/serialization/TemplateSerialization.kt`
at line 10, The PLACEHOLDER_PATTERN currently uses .+? which can capture closing
braces and allow malformed nested placeholders; update the Pattern used by
PLACEHOLDER_PATTERN in TemplateSerialization.kt to disallow '}' inside the
capture group by replacing the .+? with a negated-character class (i.e. use
[^}]+ instead of .+?) so placeholder names cannot contain closing braces, then
run tests to ensure behavior is unchanged for valid placeholders.
|
|
||
| private fun parameterFromJson(json: JsonObject): TemplateParameter<*> { | ||
| val name = json.get("name").asString | ||
| val type = ParameterType.valueOf(json.get("type").asString) |
Contributor
There was a problem hiding this comment.
Add error handling for invalid parameter type strings.
ParameterType.valueOf() throws IllegalArgumentException if the type string doesn't match any enum constant. This could happen if the JSON is manually edited or corrupted.
🛡️ Proposed fix to add error handling
private fun parameterFromJson(json: JsonObject): TemplateParameter<*> {
val name = json.get("name").asString
- val type = ParameterType.valueOf(json.get("type").asString)
+ val typeString = json.get("type").asString
+ val type = try {
+ ParameterType.valueOf(typeString)
+ } catch (e: IllegalArgumentException) {
+ throw IllegalArgumentException("Invalid parameter type '$typeString' for parameter '$name'", e)
+ }
val description = json.get("description")?.asString📝 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
| val type = ParameterType.valueOf(json.get("type").asString) | |
| private fun parameterFromJson(json: JsonObject): TemplateParameter<*> { | |
| val name = json.get("name").asString | |
| val typeString = json.get("type").asString | |
| val type = try { | |
| ParameterType.valueOf(typeString) | |
| } catch (e: IllegalArgumentException) { | |
| throw IllegalArgumentException("Invalid parameter type '$typeString' for parameter '$name'", e) | |
| } | |
| val description = json.get("description")?.asString |
🤖 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/serialization/TemplateSerialization.kt`
at line 53, The call to ParameterType.valueOf(json.get("type").asString) can
throw IllegalArgumentException for invalid strings; update the parsing in
TemplateSerialization (the block that assigns 'type') to catch
IllegalArgumentException (or validate the string against
ParameterType.values()/names()) and handle it explicitly—either set a safe
default, return a clear parsing error/exception with the invalid value, or log a
descriptive message; ensure you reference the json.get("type").asString input
and replace the direct ParameterType.valueOf call with the guarded logic in the
same method so malformed JSON doesn't cause an uncaught exception.
Comment on lines
+89
to
+98
| if (item.isJsonObject && item.asJsonObject.has("{{REPLICATE}}")) { | ||
| val replicateObj = item.asJsonObject | ||
| val listKey = replicateObj.get("{{REPLICATE}}").asString | ||
| val template = replicateObj.get("template") ?: return@forEach | ||
|
|
||
| val listData = data.get<List<Map<String, Any>>>(listKey) | ||
| listData?.forEach { itemData -> | ||
| val itemTemplateData = TemplateData(itemData) | ||
| newArray.add(applyData(template, itemTemplateData)) | ||
| } |
Contributor
There was a problem hiding this comment.
Add type validation for {{REPLICATE}} list data.
The code casts data to List<Map<String, Any>> (line 94) without validation. If the data type is incorrect (e.g., a simple list of strings or a non-list), this will throw ClassCastException at runtime.
🛡️ Proposed fix to add type validation
val listKey = replicateObj.get("{{REPLICATE}}").asString
val template = replicateObj.get("template") ?: return@forEach
- val listData = data.get<List<Map<String, Any>>>(listKey)
+ val listData = data.get<List<*>>(listKey)
+ if (listData == null) {
+ return@forEach
+ }
+
listData?.forEach { itemData ->
+ if (itemData !is Map<*, *>) {
+ return@forEach
+ }
- val itemTemplateData = TemplateData(itemData)
+ `@Suppress`("UNCHECKED_CAST")
+ val itemTemplateData = TemplateData(itemData as Map<String, Any>)
newArray.add(applyData(template, itemTemplateData))
}📝 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
| if (item.isJsonObject && item.asJsonObject.has("{{REPLICATE}}")) { | |
| val replicateObj = item.asJsonObject | |
| val listKey = replicateObj.get("{{REPLICATE}}").asString | |
| val template = replicateObj.get("template") ?: return@forEach | |
| val listData = data.get<List<Map<String, Any>>>(listKey) | |
| listData?.forEach { itemData -> | |
| val itemTemplateData = TemplateData(itemData) | |
| newArray.add(applyData(template, itemTemplateData)) | |
| } | |
| if (item.isJsonObject && item.asJsonObject.has("{{REPLICATE}}")) { | |
| val replicateObj = item.asJsonObject | |
| val listKey = replicateObj.get("{{REPLICATE}}").asString | |
| val template = replicateObj.get("template") ?: return@forEach | |
| val listData = data.get<List<*>>(listKey) | |
| if (listData == null) { | |
| return@forEach | |
| } | |
| listData?.forEach { itemData -> | |
| if (itemData !is Map<*, *>) { | |
| return@forEach | |
| } | |
| `@Suppress`("UNCHECKED_CAST") | |
| val itemTemplateData = TemplateData(itemData as Map<String, Any>) | |
| newArray.add(applyData(template, itemTemplateData)) | |
| } |
🤖 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/serialization/TemplateSerialization.kt`
around lines 89 - 98, The code assumes data.get<List<Map<String, Any>>>(listKey)
returns a List<Map<String, Any>> and will ClassCastException if it's not; update
the replicate handling (inside the block checking item.isJsonObject and
"{{REPLICATE}}") to first retrieve listData as Any?, validate it is a List<*>,
then for each element ensure the element is a Map<*,*> and safely coerce/convert
keys to String to create TemplateData (or skip non-map elements), and only then
call applyData(template, itemTemplateData) to add to newArray; reference the
symbols listKey, listData, TemplateData, applyData and the replicateObj/template
handling so you add a type guard and graceful skip/logging for invalid types
instead of an unchecked cast.
Comment on lines
+123
to
+126
| while (matcher.find()) { | ||
| val paramName = matcher.group(1) ?: "" | ||
| val value = data.get<Any>(paramName)?.toString() ?: matcher.group(0) | ||
| matcher.appendReplacement(sb, value) |
Contributor
There was a problem hiding this comment.
Escape special regex characters in replacement string.
matcher.appendReplacement(sb, value) treats $ and \ as special regex replacement characters. If the template data contains these characters (e.g., currency values like "$100"), they will be misinterpreted.
🐛 Proposed fix to escape special characters
while (matcher.find()) {
val paramName = matcher.group(1) ?: ""
val value = data.get<Any>(paramName)?.toString() ?: matcher.group(0)
- matcher.appendReplacement(sb, value)
+ matcher.appendReplacement(sb, java.util.regex.Matcher.quoteReplacement(value))
}🤖 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/serialization/TemplateSerialization.kt`
around lines 123 - 126, The replacement string passed to
matcher.appendReplacement is not escaped, so special characters like '$' and '\'
in template values (e.g., currency) will be misinterpreted; update the code in
TemplateSerialization.kt where the Matcher is used (the matcher variable and its
appendReplacement call) to wrap the computed value with a safe-escape function
(use Matcher.quoteReplacement or equivalent) before calling
matcher.appendReplacement(sb, ...), ensuring null/empty handling remains the
same.
f986f40 to
c133c30
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (4)
modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/serialization/TemplateSerialization.kt (4)
123-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape special regex characters in replacement string.
matcher.appendReplacement(sb, value)treats$and\as special regex replacement characters. Template data containing currency values like"$100"will be misinterpreted, causingIllegalArgumentExceptionor incorrect output.🐛 Proposed fix to escape special characters
while (matcher.find()) { val paramName = matcher.group(1) ?: "" val value = data.get<Any>(paramName)?.toString() ?: matcher.group(0) - matcher.appendReplacement(sb, value) + matcher.appendReplacement(sb, java.util.regex.Matcher.quoteReplacement(value)) }🤖 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/serialization/TemplateSerialization.kt` around lines 123 - 127, The replacement string passed to matcher.appendReplacement(sb, value) can contain special regex replacement chars (like $ and \) and must be escaped; update the code in TemplateSerialization.kt where matcher.find()/val value = data.get<Any>(paramName)?.toString() ?: matcher.group(0) is computed to pass an escaped replacement to matcher.appendReplacement by wrapping the computed value with Matcher.quoteReplacement(...) (or an equivalent escape step) before calling matcher.appendReplacement(sb, ...), ensuring paramName/data.get usage remains the same.
94-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd type validation for
{{REPLICATE}}list data.The code casts data to
List<Map<String, Any>>(line 94) without validation. If the data is a simple list of strings or non-list, this throwsClassCastExceptionat runtime.🛡️ Proposed fix to add type validation
val listKey = replicateObj.get("{{REPLICATE}}").asString val template = replicateObj.get("template") ?: return@forEach - val listData = data.get<List<Map<String, Any>>>(listKey) - listData?.forEach { itemData -> - val itemTemplateData = TemplateData(itemData) + val listData = data.get<List<*>>(listKey) ?: return@forEach + listData.forEach { itemData -> + if (itemData !is Map<*, *>) return@forEach + `@Suppress`("UNCHECKED_CAST") + val itemTemplateData = TemplateData(itemData as Map<String, Any>) newArray.add(applyData(template, itemTemplateData)) }🤖 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/serialization/TemplateSerialization.kt` around lines 94 - 97, The code assumes data.get<List<Map<String, Any>>>(listKey) yields a List of Maps and will ClassCastException for other shapes; update the TemplateSerialization logic around listKey so you first retrieve the raw value (e.g., val raw = data.get<Any>(listKey)), check that raw is a List<*> and then for each item validate it is either a Map<*,*> (convert keys/values to String/Any and wrap with TemplateData) or a primitive/String (wrap into a TemplateData using a single-key like "value" or as appropriate), skipping or logging invalid entries; ensure you reference and modify the block that constructs itemTemplateData and calls applyData(template, itemTemplateData) and add clear runtime type-checks and safe casts instead of the unchecked List<Map<String, Any>> cast.
51-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd error handling for invalid parameter type strings.
ParameterType.valueOf()on line 53 throwsIllegalArgumentExceptionif the type string doesn't match any enum constant. Malformed or manually-edited JSON could cause an unhandled crash.🛡️ Proposed fix to add error handling
private fun parameterFromJson(json: JsonObject): TemplateParameter<*> { val name = json.get("name").asString - val type = ParameterType.valueOf(json.get("type").asString) + val typeString = json.get("type").asString + val type = try { + ParameterType.valueOf(typeString) + } catch (e: IllegalArgumentException) { + throw IllegalArgumentException("Invalid parameter type '$typeString' for parameter '$name'", e) + } val description = json.get("description")?.asString🤖 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/serialization/TemplateSerialization.kt` around lines 51 - 66, parameterFromJson currently calls ParameterType.valueOf(json.get("type").asString) which will throw IllegalArgumentException for unknown strings; wrap that lookup in safe error handling (e.g., try/catch IllegalArgumentException or use runCatching) inside parameterFromJson, and on failure either throw a clear JsonParseException/IllegalStateException with a descriptive message including the invalid type and parameter name, or return a sensible default/skip behavior; update any callers to expect this clearer error so malformed JSON does not cause an unhandled crash.
10-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPlaceholder pattern may incorrectly match nested braces.
The regex
\\{\\{(.+?)\\}\\}uses.+?which can match content containing}characters, allowing malformed nested placeholders like{{a{{b}}to extracta{{b.🔧 Proposed fix to prevent matching closing braces
- private val PLACEHOLDER_PATTERN = Pattern.compile("\\{\\{(.+?)\\}\\}") + private val PLACEHOLDER_PATTERN = Pattern.compile("\\{\\{([^}]+)\\}\\}")🤖 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/serialization/TemplateSerialization.kt` at line 10, The PLACEHOLDER_PATTERN in TemplateSerialization.kt currently uses a dot-star non-greedy match which allows matching closing brace characters and can capture malformed/nested placeholders; update the PLACEHOLDER_PATTERN definition to replace the inner ".+?" matcher with a character-class-based matcher that excludes the closing brace (so the pattern stops at the first '}}'), ensuring placeholders like "{{a{{b}}" are not accepted; change the regex used by PLACEHOLDER_PATTERN accordingly and run tests that exercise placeholder parsing in TemplateSerialization to verify correct behavior.
🧹 Nitpick comments (4)
modules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/MotionEffectRegistrationTest.kt (1)
66-148: ⚡ Quick winAssert parsed fields for parameterized effects, not only class type.
For
ZoomInEffect,ZoomOutEffect,BlurEffect,GlitchEffect, andVibrateEffect, add assertions for parsed values (startScale/endScale,toBlurRadius,intensity,amplitude/frequency) so serializer/parser regressions are caught.🤖 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/MotionEffectRegistrationTest.kt` around lines 66 - 148, Tests only assert the runtime type from json.toMotionEffect() but do not verify that numeric fields were parsed correctly; update the relevant tests (testZoomInEffectRegistration, testZoomOutEffectRegistration, testBlurEffectRegistration, testGlitchEffectRegistration, testVibrateEffectRegistration) to cast the returned effect to the concrete class (e.g., ZoomInEffect, ZoomOutEffect, BlurEffect, GlitchEffect, VibrateEffect) and add assertions that expected properties match the JSON: for ZoomIn/ZoomOut assert startScale and endScale, for Blur assert maxBlurRadius (or toBlurRadius accessor), for Glitch assert intensity, and for Vibrate assert amplitude and frequency so serialization/parsing regressions are caught.modules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/infra/MotionTransitionParserTest.kt (1)
21-57: ⚡ Quick winAdd negative-path tests for parser contract.
Please add cases for missing
"type"and unregistered"type"to asserttoMotionTransition()throws as expected; this locks down error behavior.🤖 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/infra/MotionTransitionParserTest.kt` around lines 21 - 57, Add two negative-path unit tests to MotionTransitionParserTest: one (e.g., testMissingTypeThrows) that builds a JSON object lacking the "type" property and asserts that calling toMotionTransition() throws, and another (e.g., testUnregisteredTypeThrows) that builds a JSON with "type" set to an unknown value (like "UnknownTransition") and asserts toMotionTransition() throws; locate the test class and add the new tests alongside testCrossFadeSerialization/testBlurSerialization/testSlideSerialization, and use the project's standard assertThrows/assertFailsWith to validate the exception from toMotionTransition().modules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/MotionTemplateTest.kt (2)
20-26: 💤 Low valueContent block assertions are never executed.
The assertions inside the
contentblock (lines 24-25) will never run sincebuildContentis not invoked in this test. This makes the test appear to verify data extraction behavior when it only validates template metadata. Consider removing these assertions or adding a comment clarifying they are documentation-only placeholders.🤖 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/test/java/com/tejpratapsingh/motionlib/templates/MotionTemplateTest.kt` around lines 20 - 26, The assertions inside the content { ... } lambda never run because buildContent is not invoked; either remove those assertions (or mark them as documentation) or call the template's buildContent(...) helper to execute the content block and validate its data. Locate the test's content lambda (the content { val title = data.getString("title"); val duration = data.getInt("duration") ... }) and either delete/comment those assertEquals lines or add a call to the template's buildContent(data) / buildContent(...) method (the method used in production tests to execute the content block) so the assertions actually execute against the provided data.
68-87: 💤 Low valueContent block with assertions is not exercised.
Similar to the first test, the assertions within this
contentblock (lines 70, 74, 78, 83) and theaudio()call (line 86) are never executed. The comments at lines 108-113 acknowledge this limitation, but having unreachable assertions can be misleading for future maintainers.🤖 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/test/java/com/tejpratapsingh/motionlib/templates/MotionTemplateTest.kt` around lines 68 - 87, The assertions inside the content { ... } block and the audio(...) call are never executed; locate the content lambda in MotionTemplateTest.kt and execute it (or run the template rendering path that invokes it) using the same test fixture data so data.getString, data.get<List<LyricLine>> and audio(...) are actually exercised; if audio() has side effects, provide a test double/stub for audio or pass a mock File/frames to avoid external I/O while still invoking audio(...) so those assertions run at test time.
🤖 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/app/src/main/java/com/tejpratapsingh/animator/presentation/SampleMotionVideo.kt`:
- Around line 130-133: The method chain with addMotionViewToSequence and
addTransition violates ktlint chain-wrapping rules; reformat the chain so each
chained call starts on its own line with the leading dot aligned (e.g., place
the closing parenthesis on its own line then use lines like
`.addMotionViewToSequence(motionView = motionView)`,
`.addTransition(CrossFadeTransition(), duration = 30)`,
`.addMotionViewToSequence(motionView = motionView2)`), and remove any trailing
spaces so ktlint's chain-formatting check passes (locate the chain around
addMotionViewToSequence and addTransition in SampleMotionVideo).
In
`@modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionTransition.kt`:
- Line 17: The function signature for MotionTransition.apply should be
reformatted to satisfy ktlint's function-signature rule: break the parameters
onto multiple lines. Locate the apply method declaration in the
MotionTransition.kt file (function name: apply, class/interface:
MotionTransition) and change the single-line signature "fun apply(from:
MotionView, to: MotionView, duration: Int)" to a multiline parameter list so
each parameter (from: MotionView, to: MotionView, duration: Int) is on its own
line and the closing parenthesis and return type (if any) follow ktlint
conventions.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/LyricsTemplateSelector.kt`:
- Around line 50-51: The code assumes LyricsTemplateRegistry.templates is
non-empty and indexes templates[0], which will crash when empty; before creating
pagerState or accessing templates[0] (look for the val templates =
LyricsTemplateRegistry.templates, rememberPagerState { templates.size } and any
usages of templates[0]/templates[index]), guard for templates.isEmpty() — either
early-return a placeholder UI or create pagerState with zero pages and skip
template indexing, and ensure all downstream accesses to templates use safe
checks (e.g., only index when templates.isNotEmpty()) so no direct indexing
occurs on an empty list.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/PopupLyricsTemplate.kt`:
- Line 36: The current iteration uses lyrics.zipWithNext().slice(0..1).forEach {
(current, next) -> ... } which can throw IndexOutOfBoundsException for short
lyric sequences; replace the slice(0..1) call with take(2) on the sequence
returned by lyrics.zipWithNext() so you safely limit to up to two pairs (i.e.,
lyrics.zipWithNext().take(2).forEach { (current, next) -> ... }) and preserve
the existing forEach handling.
In
`@modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt`:
- Line 70: Guard against motionConfig.fps being zero or negative before
performing divisions: update places that compute frameDurationMs and any
time-format calculations (references: motionConfig.fps, frameDurationMs and the
time-formatting logic near lines ~203-205) to clamp or fallback the fps to a
sane positive value (e.g., use max(1, motionConfig.fps) or a default like 30)
before dividing, and optionally log/warn when fps was invalid so the code never
divides by zero or a negative number.
- Around line 105-109: The MediaPlayer creation and blocking prepare/start calls
inside LaunchedEffect should be made safe: wrap the sequence that uses
MediaPlayer (setDataSource(audio.file.absolutePath), prepare(), start()) in a
try-catch and perform prepare() on a background thread (use
withContext(Dispatchers.IO) or similar) before starting; use runCatching or
null-checks so you only add a fully-prepared/started MediaPlayer to
activePlayers, and ensure you release the MediaPlayer in the catch/finally path
on failure to avoid leaks.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/BaseMotionTransition.kt`:
- Around line 16-23: The code computes a transition window using duration in
BaseMotionTransition (variables halfDuration, transitionStart, transitionEnd)
without validating duration, which can make transitionEnd < transitionStart for
non-positive durations; add a validation at the start of the sequence (e.g., in
the method that computes halfDuration/transitionStart/transitionEnd) to require
duration > 0 (throw IllegalArgumentException or return early) and ensure
onApply(from, to, transitionStart, transitionEnd, duration) is only called when
duration is valid so the computed frame window is always well-formed.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/image/CircularMotionImageView.kt`:
- Around line 72-75: The code currently blocks the UI in CircularMotionImageView
by calling runBlocking to call the suspend ImageUtil.fetchBitmap; replace this
with a non-blocking coroutine: create a CoroutineScope tied to the view (e.g., a
property scope = CoroutineScope(SupervisorJob() + Dispatchers.Main)), then
inside the initialization use scope.launch { val bitmap =
withContext(Dispatchers.IO) { ImageUtil.fetchBitmap(context, imageUri) };
imageView.setImageBitmap(bitmap) } and cancel the scope in
onDetachedFromWindow() to avoid leaks; remove runBlocking and direct UI-thread
blocking calls.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/BlurEffect.kt`:
- Around line 34-47: The interpolated blurRadius passed into
RenderEffect.createBlurEffect can be negative and crash the API; update the
value produced by MotionInterpolator.interpolateForRange in BlurEffect (the
blurRadius variable) to be clamped to a safe minimum using .coerceAtLeast(0.1f)
before calling view.setRenderEffect(RenderEffect.createBlurEffect(...)) so the
effective radius never drops below 0.1f.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/FadeInEffect.kt`:
- Around line 21-27: The pre-range branch in the FadeIn effect (the block
guarding "if (frame !in startFrame..endFrame)") only sets view.alpha = 1f when
frame > endFrame, causing an opaque jump at start; update that branch to
explicitly set view.alpha = 0f when frame < startFrame (while keeping view.alpha
= 1f for frame > endFrame) before returning motionView so alpha is deterministic
outside the effect range (references: frame, startFrame, endFrame, view.alpha,
motionView).
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/GlitchEffect.kt`:
- Around line 18-40: The early return when frame !in startFrame..endFrame leaves
stale transforms on the view; update the method in GlitchEffect (the block using
motionView/View and startFrame..endFrame) to, before returning, reset
view.translationX, view.translationY, view.alpha, view.scaleX and view.scaleY to
their default values (0f for translations, 1f for alpha and scales) so
out-of-range frames don't retain previous glitch state.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/VibrateEffect.kt`:
- Around line 16-27: The forFrame method in VibrateEffect is returning early
when frame is outside startFrame..endFrame leaving the view's translation
offsets applied; update the logic in forFrame (in VibrateEffect) so that when
motionView is a View but frame is outside startFrame..endFrame you explicitly
reset view.translationX and view.translationY to 0f before returning, keeping
the initial motionView return behavior when motionView is not a View; ensure you
reference the same motionView cast (val view = motionView as View) and the
startFrame/endFrame range check so the reset happens only when the effect is not
active.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/utils/ImageUtil.kt`:
- Around line 24-45: Both fetchFromNetwork and fetchFromLocal currently catch
Exception and swallow CancellationException, breaking coroutine cancellation;
update each to first catch kotlinx.coroutines.CancellationException and rethrow
it, then catch other exceptions (or specific IO/Runtime exceptions) to return
null—i.e., in fetchFromNetwork and fetchFromLocal add a separate catch for
CancellationException that simply throws, then keep the general error handling
for non-cancellation failures so cancellation propagates correctly.
In
`@modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/SDUIMotionVideoProducerFactory.kt`:
- Around line 55-57: The code in SDUIMotionVideoProducerFactory.kt assumes
sdui.get("sequence") is always an array before calling asJsonArray; update the
block in the producer creation path to first verify sdui.has("sequence") and
that sdui.get("sequence") is non-null and isJsonArray (e.g., using isJsonArray)
before calling asJsonArray, and otherwise skip processing the sequence or handle
the malformed case gracefully; ensure the check surrounds the sequenceArray
assignment and the forEach usage so methods like asJsonArray are only invoked on
valid JsonArray instances.
---
Duplicate comments:
In
`@modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/serialization/TemplateSerialization.kt`:
- Around line 123-127: The replacement string passed to
matcher.appendReplacement(sb, value) can contain special regex replacement chars
(like $ and \) and must be escaped; update the code in TemplateSerialization.kt
where matcher.find()/val value = data.get<Any>(paramName)?.toString() ?:
matcher.group(0) is computed to pass an escaped replacement to
matcher.appendReplacement by wrapping the computed value with
Matcher.quoteReplacement(...) (or an equivalent escape step) before calling
matcher.appendReplacement(sb, ...), ensuring paramName/data.get usage remains
the same.
- Around line 94-97: The code assumes data.get<List<Map<String, Any>>>(listKey)
yields a List of Maps and will ClassCastException for other shapes; update the
TemplateSerialization logic around listKey so you first retrieve the raw value
(e.g., val raw = data.get<Any>(listKey)), check that raw is a List<*> and then
for each item validate it is either a Map<*,*> (convert keys/values to
String/Any and wrap with TemplateData) or a primitive/String (wrap into a
TemplateData using a single-key like "value" or as appropriate), skipping or
logging invalid entries; ensure you reference and modify the block that
constructs itemTemplateData and calls applyData(template, itemTemplateData) and
add clear runtime type-checks and safe casts instead of the unchecked
List<Map<String, Any>> cast.
- Around line 51-66: parameterFromJson currently calls
ParameterType.valueOf(json.get("type").asString) which will throw
IllegalArgumentException for unknown strings; wrap that lookup in safe error
handling (e.g., try/catch IllegalArgumentException or use runCatching) inside
parameterFromJson, and on failure either throw a clear
JsonParseException/IllegalStateException with a descriptive message including
the invalid type and parameter name, or return a sensible default/skip behavior;
update any callers to expect this clearer error so malformed JSON does not cause
an unhandled crash.
- Line 10: The PLACEHOLDER_PATTERN in TemplateSerialization.kt currently uses a
dot-star non-greedy match which allows matching closing brace characters and can
capture malformed/nested placeholders; update the PLACEHOLDER_PATTERN definition
to replace the inner ".+?" matcher with a character-class-based matcher that
excludes the closing brace (so the pattern stops at the first '}}'), ensuring
placeholders like "{{a{{b}}" are not accepted; change the regex used by
PLACEHOLDER_PATTERN accordingly and run tests that exercise placeholder parsing
in TemplateSerialization to verify correct behavior.
---
Nitpick comments:
In
`@modules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/infra/MotionTransitionParserTest.kt`:
- Around line 21-57: Add two negative-path unit tests to
MotionTransitionParserTest: one (e.g., testMissingTypeThrows) that builds a JSON
object lacking the "type" property and asserts that calling toMotionTransition()
throws, and another (e.g., testUnregisteredTypeThrows) that builds a JSON with
"type" set to an unknown value (like "UnknownTransition") and asserts
toMotionTransition() throws; locate the test class and add the new tests
alongside
testCrossFadeSerialization/testBlurSerialization/testSlideSerialization, and use
the project's standard assertThrows/assertFailsWith to validate the exception
from toMotionTransition().
In
`@modules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/MotionEffectRegistrationTest.kt`:
- Around line 66-148: Tests only assert the runtime type from
json.toMotionEffect() but do not verify that numeric fields were parsed
correctly; update the relevant tests (testZoomInEffectRegistration,
testZoomOutEffectRegistration, testBlurEffectRegistration,
testGlitchEffectRegistration, testVibrateEffectRegistration) to cast the
returned effect to the concrete class (e.g., ZoomInEffect, ZoomOutEffect,
BlurEffect, GlitchEffect, VibrateEffect) and add assertions that expected
properties match the JSON: for ZoomIn/ZoomOut assert startScale and endScale,
for Blur assert maxBlurRadius (or toBlurRadius accessor), for Glitch assert
intensity, and for Vibrate assert amplitude and frequency so
serialization/parsing regressions are caught.
In
`@modules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/MotionTemplateTest.kt`:
- Around line 20-26: The assertions inside the content { ... } lambda never run
because buildContent is not invoked; either remove those assertions (or mark
them as documentation) or call the template's buildContent(...) helper to
execute the content block and validate its data. Locate the test's content
lambda (the content { val title = data.getString("title"); val duration =
data.getInt("duration") ... }) and either delete/comment those assertEquals
lines or add a call to the template's buildContent(data) / buildContent(...)
method (the method used in production tests to execute the content block) so the
assertions actually execute against the provided data.
- Around line 68-87: The assertions inside the content { ... } block and the
audio(...) call are never executed; locate the content lambda in
MotionTemplateTest.kt and execute it (or run the template rendering path that
invokes it) using the same test fixture data so data.getString,
data.get<List<LyricLine>> and audio(...) are actually exercised; if audio() has
side effects, provide a test double/stub for audio or pass a mock File/frames to
avoid external I/O while still invoking audio(...) so those assertions run at
test time.
🪄 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: c4983e18-69e4-4dbb-9039-65b996a58ab8
📒 Files selected for processing (73)
.idea/gradle.xmlmodules/3d-filament-renderer/src/main/java/com/tejpratapsingh/motionlib/filamentrenderer/Filament3dView.ktmodules/3d-opengl-renderer/src/main/java/com/tejpratapsingh/motionlib/openglrenderer/MotionOpenGlView.ktmodules/app/src/main/java/com/tejpratapsingh/animator/presentation/SampleMotionVideo.ktmodules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionTransition.ktmodules/ffmpeg-motion-ext/src/main/java/com/tejpratapsingh/motionlib/ffmpeg/video/FFMpegVideoFrameView.ktmodules/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/LyricsTemplateSelector.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/LyricsTemplateExtensions.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/LyricsTemplateRegistry.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/PopupLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/TypewriterLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/WordwriterLyricsTemplate.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/motion-video-player/.gitignoremodules/motion-video-player/build.gradlemodules/motion-video-player/consumer-rules.promodules/motion-video-player/proguard-rules.promodules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.ktmodules/motionlib/build.gradlemodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/BaseContourMotionView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/BaseMotionTransition.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/IMotionVideoProducer.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/MotionVideoProducer.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/transitions/BlurTransition.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/transitions/CrossFadeTransition.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/transitions/SlideTransition.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/audio/BaseAudioWaveformView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/audio/CircularAudioWaveformView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/audio/RadialAudioWaveformView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/image/CircularMotionImageView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/image/MotionImageView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/BlurEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/FadeInEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/FadeOutEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/GlitchEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/SlideBottomToTopEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/SlideEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/SlideLeftToRightEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/SlideTopToBottomEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/VibrateEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/ZoomInEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/ZoomOutEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/utils/ImageUtil.ktmodules/motionlib/src/test/java/com/tejpratapsingh/motionlib/core/motion/MotionTransitionTest.ktmodules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionSdui.ktmodules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionSduiInitializer.ktmodules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionTransitionParser.ktmodules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/SDUIMotionVideoProducerFactory.ktmodules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/MotionEffectRegistrationTest.ktmodules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/infra/MotionTransitionParserTest.ktmodules/templates/README.mdmodules/templates/build.gradlemodules/templates/src/androidTest/java/com/tejpratapsingh/motionlib/templates/MotionTemplateInstrumentationTest.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/dsl/MotionTemplateDSL.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/AudioExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/BackgroundExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/ContainerExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/ImageExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/TextExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/VideoExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/json/JsonMotionTemplate.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/model/MotionTemplate.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/model/TemplateData.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/model/TemplateParameter.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/serialization/TemplateSerialization.ktmodules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/MotionTemplateTest.ktmodules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/TemplateSerializationTest.ktsettings.gradle
✅ Files skipped from review due to trivial changes (8)
- modules/motion-video-player/.gitignore
- .idea/gradle.xml
- modules/motion-video-player/proguard-rules.pro
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/ZoomInEffect.kt
- modules/motion-video-player/build.gradle
- modules/3d-filament-renderer/src/main/java/com/tejpratapsingh/motionlib/filamentrenderer/Filament3dView.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/transitions/SlideTransition.kt
- modules/templates/README.md
🚧 Files skipped from review as they are similar to previous changes (13)
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/BackgroundExtensions.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/model/TemplateParameter.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/model/MotionTemplate.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/model/TemplateData.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/AudioExtensions.kt
- modules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/TemplateSerializationTest.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/ImageExtensions.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/MotionVideoProducer.kt
- modules/templates/src/androidTest/java/com/tejpratapsingh/motionlib/templates/MotionTemplateInstrumentationTest.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/VideoExtensions.kt
- modules/templates/build.gradle
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/ContainerExtensions.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/TextExtensions.kt
| * @param to The incoming view. | ||
| * @param duration The duration of the transition in frames. | ||
| */ | ||
| fun apply(from: MotionView, to: MotionView, duration: Int) |
Contributor
There was a problem hiding this comment.
Wrap apply parameters per ktlint function-signature rule.
Line 17 needs multiline parameter formatting; current signature fails style checks.
Proposed fix
- fun apply(from: MotionView, to: MotionView, duration: Int)
+ fun apply(
+ from: MotionView,
+ to: MotionView,
+ duration: Int,
+ )📝 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
| fun apply(from: MotionView, to: MotionView, duration: Int) | |
| fun apply( | |
| from: MotionView, | |
| to: MotionView, | |
| duration: Int, | |
| ) |
🧰 Tools
🪛 GitHub Check: ktlint
[failure] 17-17:
Parameter should start on a newline
[failure] 17-17:
Parameter should start on a newline
[failure] 17-17:
Newline expected after opening parenthesis
🤖 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/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionTransition.kt`
at line 17, The function signature for MotionTransition.apply should be
reformatted to satisfy ktlint's function-signature rule: break the parameters
onto multiple lines. Locate the apply method declaration in the
MotionTransition.kt file (function name: apply, class/interface:
MotionTransition) and change the single-line signature "fun apply(from:
MotionView, to: MotionView, duration: Int)" to a multiline parameter list so
each parameter (from: MotionView, to: MotionView, duration: Int) is on its own
line and the closing parenthesis and return type (if any) follow ktlint
conventions.
Comment on lines
+50
to
+51
| val templates = LyricsTemplateRegistry.templates | ||
| val pagerState = rememberPagerState { templates.size } |
Contributor
There was a problem hiding this comment.
Guard empty template list before indexing current page.
Line 109 can crash when templates is empty (templates[0]).
Proposed fix
val templates = LyricsTemplateRegistry.templates
val pagerState = rememberPagerState { templates.size }
+ val hasTemplates = templates.isNotEmpty()
@@
Button(
onClick = {
- onTemplateSelected(templates[pagerState.currentPage])
+ if (hasTemplates) {
+ onTemplateSelected(templates[pagerState.currentPage])
+ }
},
+ enabled = hasTemplates,
modifier =
Modifier
.fillMaxWidth()Also applies to: 107-110
🤖 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/LyricsTemplateSelector.kt`
around lines 50 - 51, The code assumes LyricsTemplateRegistry.templates is
non-empty and indexes templates[0], which will crash when empty; before creating
pagerState or accessing templates[0] (look for the val templates =
LyricsTemplateRegistry.templates, rememberPagerState { templates.size } and any
usages of templates[0]/templates[index]), guard for templates.isEmpty() — either
early-return a placeholder UI or create pagerState with zero pages and skip
template indexing, and ensure all downstream accesses to templates use safe
checks (e.g., only index when templates.isNotEmpty()) so no direct indexing
occurs on an empty list.
| image = image, | ||
| ) | ||
|
|
||
| lyrics.zipWithNext().slice(0..1).forEach { (current, next) -> |
Contributor
There was a problem hiding this comment.
Prevent runtime crash on short lyrics sequences.
Line 36 can throw IndexOutOfBoundsException when lyrics.zipWithNext() has fewer than 2 pairs (e.g., 1–2 lyric lines). Use take(2) instead of slice(0..1).
💡 Proposed fix
- lyrics.zipWithNext().slice(0..1).forEach { (current, next) ->
+ lyrics.zipWithNext().take(2).forEach { (current, next) ->🤖 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/PopupLyricsTemplate.kt`
at line 36, The current iteration uses lyrics.zipWithNext().slice(0..1).forEach
{ (current, next) -> ... } which can throw IndexOutOfBoundsException for short
lyric sequences; replace the slice(0..1) call with take(2) on the sequence
returned by lyrics.zipWithNext() so you safely limit to up to two pairs (i.e.,
lyrics.zipWithNext().take(2).forEach { (current, next) -> ... }) and preserve
the existing forEach handling.
| // Playback loop | ||
| LaunchedEffect(isPlaying) { | ||
| if (isPlaying) { | ||
| val frameDurationMs = (1000.0 / motionConfig.fps).toLong() |
Contributor
There was a problem hiding this comment.
Guard FPS before division to prevent runtime crashes.
Line 70 and Line 204 divide by fps without validation. If config/template data ever provides fps <= 0, playback timing and time formatting will crash or misbehave.
Proposed fix
- val frameDurationMs = (1000.0 / motionConfig.fps).toLong()
+ val safeFps = motionConfig.fps.coerceAtLeast(1)
+ val frameDurationMs = (1000.0 / safeFps).toLong()
...
- text = "${formatTime(currentFrame, motionConfig.fps)} / ${formatTime(totalFrames, motionConfig.fps)}",
+ text = "${formatTime(currentFrame, safeFps)} / ${formatTime(totalFrames, safeFps)}",
...
private fun formatTime(frames: Int, fps: Int): String {
+ if (fps <= 0) return "00:00"
val totalSeconds = frames / fpsAlso applies to: 203-205
🤖 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`
at line 70, Guard against motionConfig.fps being zero or negative before
performing divisions: update places that compute frameDurationMs and any
time-format calculations (references: motionConfig.fps, frameDurationMs and the
time-formatting logic near lines ~203-205) to clamp or fallback the fps to a
sane positive value (e.g., use max(1, motionConfig.fps) or a default like 30)
before dividing, and optionally log/warn when fps was invalid so the code never
divides by zero or a negative number.
Comment on lines
+21
to
+27
| if (frame !in startFrame..endFrame) { | ||
| // If we are past the effect, ensure alpha is 1 | ||
| if (frame > endFrame) { | ||
| view.alpha = 1f | ||
| } | ||
| return motionView | ||
| } |
Contributor
There was a problem hiding this comment.
Pre-start alpha should be explicitly set to 0f for fade-in.
Current pre-range behavior can cause a visible jump at effect start (opaque before start, then sudden drop to 0 at start frame).
Deterministic frame-range handling
- if (frame !in startFrame..endFrame) {
- // If we are past the effect, ensure alpha is 1
- if (frame > endFrame) {
- view.alpha = 1f
- }
- return motionView
- }
+ if (frame < startFrame) {
+ view.alpha = 0f
+ return motionView
+ }
+ if (frame > endFrame) {
+ view.alpha = 1f
+ return motionView
+ }🤖 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/effects/FadeInEffect.kt`
around lines 21 - 27, The pre-range branch in the FadeIn effect (the block
guarding "if (frame !in startFrame..endFrame)") only sets view.alpha = 1f when
frame > endFrame, causing an opaque jump at start; update that branch to
explicitly set view.alpha = 0f when frame < startFrame (while keeping view.alpha
= 1f for frame > endFrame) before returning motionView so alpha is deterministic
outside the effect range (references: frame, startFrame, endFrame, view.alpha,
motionView).
Comment on lines
+18
to
+40
| if (frame !in startFrame..endFrame) return motionView | ||
|
|
||
| val view = motionView as View | ||
|
|
||
| // Jitter translation | ||
| view.translationX = (random.nextFloat() * 2 - 1) * intensity | ||
| view.translationY = (random.nextFloat() * 2 - 1) * intensity | ||
|
|
||
| // Occasional alpha drop | ||
| if (random.nextFloat() > 0.8f) { | ||
| view.alpha = random.nextFloat() * 0.5f + 0.5f | ||
| } else { | ||
| view.alpha = 1f | ||
| } | ||
|
|
||
| // Occasional scale jitter | ||
| if (random.nextFloat() > 0.9f) { | ||
| view.scaleX = 1f + (random.nextFloat() * 2 - 1) * 0.1f | ||
| view.scaleY = 1f + (random.nextFloat() * 2 - 1) * 0.1f | ||
| } else { | ||
| view.scaleX = 1f | ||
| view.scaleY = 1f | ||
| } |
Contributor
There was a problem hiding this comment.
Reset transformed properties when the effect is out of range.
Line 18 exits early, so post-effect frames can retain stale translation/scale/alpha from the last glitch frame.
State reset fix
- if (motionView !is View) return motionView
- if (frame !in startFrame..endFrame) return motionView
-
- val view = motionView as View
+ if (motionView !is View) return motionView
+ val view = motionView as View
+ if (frame !in startFrame..endFrame) {
+ view.translationX = 0f
+ view.translationY = 0f
+ view.alpha = 1f
+ view.scaleX = 1f
+ view.scaleY = 1f
+ return motionView
+ }🤖 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/effects/GlitchEffect.kt`
around lines 18 - 40, The early return when frame !in startFrame..endFrame
leaves stale transforms on the view; update the method in GlitchEffect (the
block using motionView/View and startFrame..endFrame) to, before returning,
reset view.translationX, view.translationY, view.alpha, view.scaleX and
view.scaleY to their default values (0f for translations, 1f for alpha and
scales) so out-of-range frames don't retain previous glitch state.
Comment on lines
+16
to
+27
| override fun forFrame(frame: Int): MotionView { | ||
| if (motionView !is View) return motionView | ||
| if (frame !in startFrame..endFrame) return motionView | ||
|
|
||
| val view = motionView as View | ||
|
|
||
| // Using sin wave for vibration | ||
| val offset = sin(frame.toDouble() * frequency).toFloat() * amplitude | ||
|
|
||
| view.translationX = offset | ||
| view.translationY = offset / 2f // Slight diagonal vibration | ||
|
|
Contributor
There was a problem hiding this comment.
Reset vibration offsets outside the active frame range.
When the frame exits startFrame..endFrame, the code returns early and keeps the last vibration offset, so the view can stay permanently shifted.
Proposed fix
override fun forFrame(frame: Int): MotionView {
if (motionView !is View) return motionView
- if (frame !in startFrame..endFrame) return motionView
-
val view = motionView as View
+ if (frame !in startFrame..endFrame) {
+ view.translationX = 0f
+ view.translationY = 0f
+ return motionView
+ }
// Using sin wave for vibration
val offset = sin(frame.toDouble() * frequency).toFloat() * amplitude📝 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
| override fun forFrame(frame: Int): MotionView { | |
| if (motionView !is View) return motionView | |
| if (frame !in startFrame..endFrame) return motionView | |
| val view = motionView as View | |
| // Using sin wave for vibration | |
| val offset = sin(frame.toDouble() * frequency).toFloat() * amplitude | |
| view.translationX = offset | |
| view.translationY = offset / 2f // Slight diagonal vibration | |
| override fun forFrame(frame: Int): MotionView { | |
| if (motionView !is View) return motionView | |
| val view = motionView as View | |
| if (frame !in startFrame..endFrame) { | |
| view.translationX = 0f | |
| view.translationY = 0f | |
| return motionView | |
| } | |
| // Using sin wave for vibration | |
| val offset = sin(frame.toDouble() * frequency).toFloat() * amplitude | |
| view.translationX = offset | |
| view.translationY = offset / 2f // Slight diagonal vibration | |
🤖 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/effects/VibrateEffect.kt`
around lines 16 - 27, The forFrame method in VibrateEffect is returning early
when frame is outside startFrame..endFrame leaving the view's translation
offsets applied; update the logic in forFrame (in VibrateEffect) so that when
motionView is a View but frame is outside startFrame..endFrame you explicitly
reset view.translationX and view.translationY to 0f before returning, keeping
the initial motionView return behavior when motionView is not a View; ensure you
reference the same motionView cast (val view = motionView as View) and the
startFrame/endFrame range check so the reset happens only when the effect is not
active.
Comment on lines
+24
to
+45
| private fun fetchFromNetwork(url: String): Bitmap? { | ||
| val request = Request.Builder().url(url).build() | ||
| return try { | ||
| client.newCall(request).execute().use { response -> | ||
| if (!response.isSuccessful) return null | ||
| val bytes = response.body()?.bytes() ?: return null | ||
| BitmapFactory.decodeByteArray(bytes, 0, bytes.size) | ||
| } | ||
| } catch (e: Exception) { | ||
| null | ||
| } | ||
| } | ||
|
|
||
| private fun fetchFromLocal(context: Context, uri: Uri): Bitmap? { | ||
| return try { | ||
| val inputStream: InputStream? = context.contentResolver.openInputStream(uri) | ||
| inputStream.use { | ||
| BitmapFactory.decodeStream(it) | ||
| } | ||
| } catch (e: Exception) { | ||
| null | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/utils/ImageUtil.kt | head -60Repository: tejpratap46/AndroidVideoMotion
Length of output: 1928
Preserve coroutine cancellation instead of swallowing it.
Lines 32 and 43 catch Exception, which also catches CancellationException. Since these functions are called from within the suspend context of fetchBitmap() (via withContext(Dispatchers.IO)), swallowing CancellationException prevents proper cancellation propagation and breaks the coroutine cancellation contract.
Catch CancellationException separately and rethrow it before catching other exceptions:
Proposed fix
import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.withContext
@@
- } catch (e: Exception) {
+ } catch (e: CancellationException) {
+ throw e
+ } catch (e: Exception) {
null
}
@@
- } catch (e: Exception) {
+ } catch (e: CancellationException) {
+ throw e
+ } catch (e: Exception) {
null
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private fun fetchFromNetwork(url: String): Bitmap? { | |
| val request = Request.Builder().url(url).build() | |
| return try { | |
| client.newCall(request).execute().use { response -> | |
| if (!response.isSuccessful) return null | |
| val bytes = response.body()?.bytes() ?: return null | |
| BitmapFactory.decodeByteArray(bytes, 0, bytes.size) | |
| } | |
| } catch (e: Exception) { | |
| null | |
| } | |
| } | |
| private fun fetchFromLocal(context: Context, uri: Uri): Bitmap? { | |
| return try { | |
| val inputStream: InputStream? = context.contentResolver.openInputStream(uri) | |
| inputStream.use { | |
| BitmapFactory.decodeStream(it) | |
| } | |
| } catch (e: Exception) { | |
| null | |
| } | |
| private fun fetchFromNetwork(url: String): Bitmap? { | |
| val request = Request.Builder().url(url).build() | |
| return try { | |
| client.newCall(request).execute().use { response -> | |
| if (!response.isSuccessful) return null | |
| val bytes = response.body()?.bytes() ?: return null | |
| BitmapFactory.decodeByteArray(bytes, 0, bytes.size) | |
| } | |
| } catch (e: CancellationException) { | |
| throw e | |
| } catch (e: Exception) { | |
| null | |
| } | |
| } | |
| private fun fetchFromLocal(context: Context, uri: Uri): Bitmap? { | |
| return try { | |
| val inputStream: InputStream? = context.contentResolver.openInputStream(uri) | |
| inputStream.use { | |
| BitmapFactory.decodeStream(it) | |
| } | |
| } catch (e: CancellationException) { | |
| throw e | |
| } catch (e: Exception) { | |
| null | |
| } | |
| } |
🤖 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/utils/ImageUtil.kt`
around lines 24 - 45, Both fetchFromNetwork and fetchFromLocal currently catch
Exception and swallow CancellationException, breaking coroutine cancellation;
update each to first catch kotlinx.coroutines.CancellationException and rethrow
it, then catch other exceptions (or specific IO/Runtime exceptions) to return
null—i.e., in fetchFromNetwork and fetchFromLocal add a separate catch for
CancellationException that simply throws, then keep the general error handling
for non-cancellation failures so cancellation propagates correctly.
Comment on lines
+55
to
+57
| if (sdui.has("sequence")) { | ||
| val sequenceArray = sdui.get("sequence").asJsonArray | ||
| sequenceArray.forEach { item -> |
Contributor
There was a problem hiding this comment.
Guard sequence type before calling asJsonArray.
Line [55]-Line [57] assumes "sequence" is always an array. Malformed/null payloads will throw and abort producer creation.
Proposed fix
- if (sdui.has("sequence")) {
- val sequenceArray = sdui.get("sequence").asJsonArray
+ val sequenceElement = sdui.get("sequence")
+ if (sequenceElement?.isJsonArray == true) {
+ val sequenceArray = sequenceElement.asJsonArray
sequenceArray.forEach { item ->
val json = item.asJsonObject
val type = json.get("type")?.asString
@@
- } else {
+ } else {
+ if (sdui.has("sequence")) {
+ Timber.w("Ignoring non-array `sequence` in SDUI payload")
+ }
views.forEach { view ->
if (view is ViewGroup) {🤖 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/main/java/com/tejpratapsingh/motion/sdui/infra/SDUIMotionVideoProducerFactory.kt`
around lines 55 - 57, The code in SDUIMotionVideoProducerFactory.kt assumes
sdui.get("sequence") is always an array before calling asJsonArray; update the
block in the producer creation path to first verify sdui.has("sequence") and
that sdui.get("sequence") is non-null and isJsonArray (e.g., using isJsonArray)
before calling asJsonArray, and otherwise skip processing the sequence or handle
the malformed case gracefully; ensure the check surrounds the sequenceArray
assignment and the forEach usage so methods like asJsonArray are only invoked on
valid JsonArray instances.
11d9e2e to
2463304
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/web-sdui/src/views/ViewRegistry.tsx (1)
28-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix sentinel handling for open-ended views in visibility check.
At Line 32,
currentFrame > props.endFramehides views withendFrame = -1(open-ended), so they never render after frame-1.Suggested fix
export const MotionViewRenderer: React.FC<{ props: MotionViewProps; currentFrame: number; config: MotionConfig }> = ({ props, currentFrame, config }) => { const Component = ViewRegistry[props.type]; // Visibility check - if (currentFrame < props.startFrame || currentFrame > props.endFrame) { + const isAfterEnd = props.endFrame !== -1 && currentFrame > props.endFrame; + if (currentFrame < props.startFrame || isAfterEnd) { return null; }🤖 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 `@web/web-sdui/src/views/ViewRegistry.tsx` around lines 28 - 34, The visibility check in MotionViewRenderer incorrectly hides open-ended views because props.endFrame == -1 causes currentFrame > props.endFrame to be true; update the condition to treat -1 as "no end" by returning null only when currentFrame < props.startFrame or (props.endFrame !== -1 && currentFrame > props.endFrame), so views with endFrame === -1 remain visible after their startFrame.
♻️ Duplicate comments (1)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/PopupLyricsTemplate.kt (1)
35-35:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReplace fixed-range
slice(0..1)with a safe limiter.Line 35 can throw
IndexOutOfBoundsExceptionwhenzipWithNext()returns fewer than 2 pairs. Usetake(2)instead.Proposed fix
- lyrics.zipWithNext().slice(0..1).forEach { (current, next) -> + lyrics.zipWithNext().take(2).forEach { (current, next) ->🤖 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/PopupLyricsTemplate.kt` at line 35, The code using lyrics.zipWithNext().slice(0..1).forEach { (current, next) -> ... } can throw IndexOutOfBoundsException when fewer than two pairs exist; replace the fixed-range slice with a safe limiter by using take(2) on the collection/sequence returned by zipWithNext() (i.e., lyrics.zipWithNext().take(2).forEach { (current, next) -> ... }) so you only iterate up to two pairs without risking an out-of-bounds access in PopupLyricsTemplate.kt.
🤖 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/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionTextSizeProvider.kt`:
- Line 39: The public mutable variable baseTextScale on MotionTextSizeProvider
is unbounded and can be set to <= 0; change it to enforce valid bounds by making
baseTextScale use a custom setter (or back it with a private var) that clamps or
rejects invalid values (e.g., ensure value > 0 and optionally cap at a
reasonable max) and either throw IllegalArgumentException for invalid inputs or
clamp to a safe range; update any call sites to set baseTextScale via the setter
to ensure the validation is applied.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/GlitchLyricsTemplate.kt`:
- Around line 35-52: The current use of lyrics.zipWithNext() in the
GlitchLyricsTemplate skips rendering when there is only one lyric; update the
rendering logic around the zipWithNext() loop to detect lyrics.size == 1 (and
similarly for previewLyrics.size == 1) and emit a single wordBlinkTextView for
that lone item using its frame as startFrame and a sensible endFrame (e.g., same
frame or frame + small delta), applying the same parameters (textSizeVariant,
textColor, effects: GlitchEffect) and the same textView padding/gravity block,
and keep the existing transition/SlideTransition usage so single-lyric inputs
produce visible output.
In
`@modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt`:
- Around line 97-123: The audio players must be seeked to the current timeline
position before any start(): inside the LaunchedEffect handling
motionVideoProducer.motionAudio, compute the target position ms = ((currentFrame
- audio.delayFrame) * 1000) / fps (clamp to >=0) and call player.seekTo(ms)
before calling start() for both newly created MediaPlayer (created in the block
where activePlayers[audio] is null) and for existing paused players (when player
!= null and !player.isPlaying) so that playback aligns with the currentFrame;
ensure you use the existing activePlayers map, currentFrame, audio.delayFrame
and fps variables and call seekTo prior to start().
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/abstract/AbstractMotionTextView.kt`:
- Around line 54-59: Replace the broad Exception catch around the textColor
parsing with a specific IllegalArgumentException handler: in the textColor?.let
{ ... this.setTextColor(it.toColorInt()) ... } block catch only
IllegalArgumentException from toColorInt(), log the error via Timber (include
the invalid color string and a message about falling back to default) and keep
the fallback behavior; remove the generic catch so other runtime errors surface.
In
`@modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionSduiInitializer.kt`:
- Line 47: The current parsing uses MotionTextVariant.valueOf(...) which throws
on unknown payload values and can crash view creation; replace these calls (the
ones setting textSizeVariant and the other similar enum parses) with a safe
lookup that returns null or a default if the incoming string is unrecognized
(e.g., use MotionTextVariant.values().firstOrNull { it.name == incomingString }
or wrap in runCatching and fallback), and update downstream code to handle the
nullable/default variant instead of allowing an exception to propagate from
MotionTextVariant.valueOf.
In `@web/web-sdui/src/views/GradientView.tsx`:
- Around line 9-12: The current orientation handling in GradientView.tsx treats
any non-'VERTICAL' orientation as a 90deg linear gradient, breaking explicit
'CIRCULAR' behavior; update the logic around the
orientation/gradientAngle/background (the variables named orientation,
gradientAngle, and background) to branch explicitly: keep 'VERTICAL' => linear
180deg, handle 'HORIZONTAL' (or other linear orientations) => linear 90deg, and
implement 'CIRCULAR' to produce a radial gradient (e.g., use a radial-gradient
with the colors array) instead of a linear-gradient so circular gradients render
correctly.
---
Outside diff comments:
In `@web/web-sdui/src/views/ViewRegistry.tsx`:
- Around line 28-34: The visibility check in MotionViewRenderer incorrectly
hides open-ended views because props.endFrame == -1 causes currentFrame >
props.endFrame to be true; update the condition to treat -1 as "no end" by
returning null only when currentFrame < props.startFrame or (props.endFrame !==
-1 && currentFrame > props.endFrame), so views with endFrame === -1 remain
visible after their startFrame.
---
Duplicate comments:
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/PopupLyricsTemplate.kt`:
- Line 35: The code using lyrics.zipWithNext().slice(0..1).forEach { (current,
next) -> ... } can throw IndexOutOfBoundsException when fewer than two pairs
exist; replace the fixed-range slice with a safe limiter by using take(2) on the
collection/sequence returned by zipWithNext() (i.e.,
lyrics.zipWithNext().take(2).forEach { (current, next) -> ... }) so you only
iterate up to two pairs without risking an out-of-bounds access in
PopupLyricsTemplate.kt.
🪄 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: c32bc8be-003c-45bb-9031-ca34b0588d49
📒 Files selected for processing (98)
.idea/gradle.xmlmodules/3d-filament-renderer/src/main/java/com/tejpratapsingh/motionlib/filamentrenderer/Filament3dView.ktmodules/3d-opengl-renderer/src/main/java/com/tejpratapsingh/motionlib/openglrenderer/MotionOpenGlView.ktmodules/app/src/main/java/com/tejpratapsingh/animator/presentation/SampleMotionVideo.ktmodules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionTextSizeProvider.ktmodules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionTransition.ktmodules/core/src/test/java/com/tejpratapsingh/motionlib/core/MotionTextSizeProviderTest.ktmodules/ffmpeg-motion-ext/src/main/java/com/tejpratapsingh/motionlib/ffmpeg/video/FFMpegVideoFrameView.ktmodules/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/LyricsTemplateSelector.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/GlitchLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/GradientLyricsTemplate.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/LyricsTemplateExtensions.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/LyricsTemplateRegistry.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/PopupLyricsTemplate.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/view/SongNameTextView.ktmodules/motion-video-player/.gitignoremodules/motion-video-player/build.gradlemodules/motion-video-player/consumer-rules.promodules/motion-video-player/proguard-rules.promodules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.ktmodules/motionlib/build.gradlemodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/BaseContourMotionView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/BaseMotionTransition.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/IMotionVideoProducer.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/MotionVideoProducer.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/transitions/BlurTransition.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/transitions/CrossFadeTransition.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/transitions/SlideTransition.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/audio/BaseAudioWaveformView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/audio/CircularAudioWaveformView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/audio/RadialAudioWaveformView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/image/CircularMotionImageView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/image/MotionImageView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/PopUpTextView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/TransparentTextView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/TypeWriterTextView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/WordBlinkTextView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/WordWriterTextView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/abstract/AbstractMotionTextView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/BlurEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/FadeInEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/FadeOutEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/GlitchEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/SlideBottomToTopEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/SlideEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/SlideLeftToRightEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/SlideTopToBottomEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/VibrateEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/ZoomInEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/ZoomOutEffect.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/utils/ImageUtil.ktmodules/motionlib/src/test/java/com/tejpratapsingh/motionlib/core/motion/MotionTransitionTest.ktmodules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionSdui.ktmodules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionSduiInitializer.ktmodules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionTransitionParser.ktmodules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/SDUIMotionVideoProducerFactory.ktmodules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/MotionEffectRegistrationTest.ktmodules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/infra/MotionTransitionParserTest.ktmodules/templates/README.mdmodules/templates/build.gradlemodules/templates/src/androidTest/java/com/tejpratapsingh/motionlib/templates/MotionTemplateInstrumentationTest.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/dsl/MotionTemplateDSL.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/AudioExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/BackgroundExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/ContainerExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/ImageExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/TextExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/VideoExtensions.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/json/JsonMotionTemplate.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/model/MotionTemplate.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/model/TemplateData.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/model/TemplateParameter.ktmodules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/serialization/TemplateSerialization.ktmodules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/MotionTemplateTest.ktmodules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/TemplateSerializationTest.ktsettings.gradleweb/web-sdui/src/components/MotionCanvas.tsxweb/web-sdui/src/effects/useApplyEffects.tsweb/web-sdui/src/views/AudioWaveformView.tsxweb/web-sdui/src/views/GradientView.tsxweb/web-sdui/src/views/MotionImageView.tsxweb/web-sdui/src/views/MultiLyricsContainer.tsxweb/web-sdui/src/views/PopUpTextView.tsxweb/web-sdui/src/views/TransparentTextView.tsxweb/web-sdui/src/views/VideoFrameView.tsxweb/web-sdui/src/views/ViewRegistry.tsxweb/web-sdui/src/views/WordBlinkTextView.tsxweb/web-sdui/src/views/WordWriterTextView.tsx
✅ Files skipped from review due to trivial changes (7)
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/transitions/CrossFadeTransition.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt
- modules/motion-video-player/.gitignore
- .idea/gradle.xml
- modules/motion-video-player/proguard-rules.pro
- modules/motionlib/build.gradle
- modules/templates/README.md
🚧 Files skipped from review as they are similar to previous changes (55)
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/LyricsTemplateRegistry.kt
- modules/lyrics-maker/build.gradle
- modules/ffmpeg-motion-ext/src/main/java/com/tejpratapsingh/motionlib/ffmpeg/video/FFMpegVideoFrameView.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/audio/RadialAudioWaveformView.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/IMotionVideoProducer.kt
- modules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/MotionEffectRegistrationTest.kt
- settings.gradle
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/VibrateEffect.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/model/TemplateParameter.kt
- modules/motion-video-player/build.gradle
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/SlideTopToBottomEffect.kt
- modules/motionlib/src/test/java/com/tejpratapsingh/motionlib/core/motion/MotionTransitionTest.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/BaseContourMotionView.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/LyricsTemplateExtensions.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/ContainerExtensions.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/AudioExtensions.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/audio/CircularAudioWaveformView.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/BaseMotionTransition.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/GlitchEffect.kt
- modules/sdui/src/test/java/com/tejpratapsingh/motion/sdui/infra/MotionTransitionParserTest.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/MultiLyricsContainer.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/BackgroundExtensions.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/model/TemplateData.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/ZoomInEffect.kt
- modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionTransitionParser.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/utils/ImageUtil.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsCompose.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/model/MotionTemplate.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/SlideLeftToRightEffect.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/FadeOutEffect.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/SlideBottomToTopEffect.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/WordwriterLyricsTemplate.kt
- modules/3d-filament-renderer/src/main/java/com/tejpratapsingh/motionlib/filamentrenderer/Filament3dView.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/LyricsTemplateSelector.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/VideoExtensions.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/templates/TypewriterLyricsTemplate.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/BlurEffect.kt
- modules/3d-opengl-renderer/src/main/java/com/tejpratapsingh/motionlib/openglrenderer/MotionOpenGlView.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/transitions/BlurTransition.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/extensions/ImageExtensions.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/dsl/MotionTemplateDSL.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/ZoomOutEffect.kt
- modules/templates/build.gradle
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/FadeInEffect.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/transitions/SlideTransition.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/SlideEffect.kt
- modules/templates/src/androidTest/java/com/tejpratapsingh/motionlib/templates/MotionTemplateInstrumentationTest.kt
- modules/sdui/src/main/java/com/tejpratapsingh/motion/sdui/infra/MotionSdui.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/MotionVideoProducer.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/image/MotionImageView.kt
- modules/templates/src/main/java/com/tejpratapsingh/motionlib/templates/json/JsonMotionTemplate.kt
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/image/CircularMotionImageView.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/AppNavHost.kt
- modules/templates/src/test/java/com/tejpratapsingh/motionlib/templates/MotionTemplateTest.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.kt
| /** | ||
| * Base scale for all font sizes. Can be updated to scale all text sizes at once. | ||
| */ | ||
| var baseTextScale: Float = 1.5f |
Contributor
There was a problem hiding this comment.
Validate baseTextScale bounds.
Line [39] exposes global mutable scale without bounds checks; <= 0 can collapse or invert text sizes throughout the pipeline.
Proposed fix
object MotionTextSizeProvider {
@@
- var baseTextScale: Float = 1.5f
+ var baseTextScale: Float = 1.5f
+ set(value) {
+ field = value.coerceAtLeast(0.1f)
+ }📝 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 baseTextScale: Float = 1.5f | |
| var baseTextScale: Float = 1.5f | |
| set(value) { | |
| field = value.coerceAtLeast(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/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionTextSizeProvider.kt`
at line 39, The public mutable variable baseTextScale on MotionTextSizeProvider
is unbounded and can be set to <= 0; change it to enforce valid bounds by making
baseTextScale use a custom setter (or back it with a private var) that clamps or
rejects invalid values (e.g., ensure value > 0 and optionally cap at a
reasonable max) and either throw IllegalArgumentException for invalid inputs or
clamp to a safe range; update any call sites to set baseTextScale via the setter
to ensure the validation is applied.
Comment on lines
+35
to
+52
| lyrics.zipWithNext().forEach { (current, next) -> | ||
| wordBlinkTextView( | ||
| text = current.text, | ||
| startFrame = current.frame, | ||
| endFrame = next.frame, | ||
| textSizeVariant = MotionTextVariant.H1, | ||
| textColor = "#FFFFFF", | ||
| effects = listOf(GlitchEffect(current.frame, next.frame, intensity = 30f)), | ||
| ) { | ||
| textView.apply { | ||
| setPadding(16, 16, 16, 16) | ||
| textAlignment = AppCompatTextView.TEXT_ALIGNMENT_CENTER | ||
| gravity = Gravity.CENTER | ||
| } | ||
| } | ||
| transition(SlideTransition(SlideDirection.BOTTOM_TO_TOP), duration = 5) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Handle single-lyric inputs so text still renders.
At Line 35 and Line 70, zipWithNext() skips rendering when there is only one lyric frame, so no wordBlinkTextView is emitted.
A simple fallback for lyrics.size == 1 (and previewLyrics.size == 1) to emit one wordBlinkTextView would avoid empty output.
Also applies to: 70-87
🤖 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/GlitchLyricsTemplate.kt`
around lines 35 - 52, The current use of lyrics.zipWithNext() in the
GlitchLyricsTemplate skips rendering when there is only one lyric; update the
rendering logic around the zipWithNext() loop to detect lyrics.size == 1 (and
similarly for previewLyrics.size == 1) and emit a single wordBlinkTextView for
that lone item using its frame as startFrame and a sensible endFrame (e.g., same
frame or frame + small delta), applying the same parameters (textSizeVariant,
textColor, effects: GlitchEffect) and the same textView padding/gravity block,
and keep the existing transition/SlideTransition usage so single-lyric inputs
produce visible output.
Comment on lines
+97
to
+123
| LaunchedEffect(currentFrame, isPlaying) { | ||
| if (isPlaying) { | ||
| motionVideoProducer.motionAudio.forEach { audio -> | ||
| val shouldPlay = currentFrame in audio.delayFrame..audio.endFrame | ||
| val player = activePlayers[audio] | ||
|
|
||
| if (shouldPlay) { | ||
| if (player == null) { | ||
| val mediaPlayer = MediaPlayer().apply { | ||
| setDataSource(audio.file.absolutePath) | ||
| prepare() | ||
| start() | ||
| } | ||
| activePlayers[audio] = mediaPlayer | ||
| } else if (!player.isPlaying) { | ||
| player.start() | ||
| } | ||
| } else { | ||
| if (player != null) { | ||
| if (player.isPlaying) player.pause() | ||
| player.release() | ||
| activePlayers.remove(audio) | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| activePlayers.values.forEach { if (it.isPlaying) it.pause() } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file and check its size
find . -name "MotionVideoPlayerCompose.kt" -type fRepository: tejpratap46/AndroidVideoMotion
Length of output: 191
🏁 Script executed:
# Read the full file to understand the context
wc -l modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 193
🏁 Script executed:
# Get the context around the LaunchedEffect block (lines 97-123 and surrounding)
sed -n '80,140p' modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 2046
🏁 Script executed:
# Search for motionConfig and fps usage to verify the proposed fix's accessibility
rg "motionConfig" modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.kt -B 5 -A 5Repository: tejpratap46/AndroidVideoMotion
Length of output: 1754
🏁 Script executed:
# Check the audio data class structure to understand delayFrame and endFrame
rg -t kotlin "class.*Audio|data class.*Audio" modules/motion-video-player/ -A 10Repository: tejpratap46/AndroidVideoMotion
Length of output: 56
🏁 Script executed:
# Search for Audio class more specifically across the motion-video-player module
find modules/motion-video-player -name "*.kt" -type f | xargs rg -l "delayFrame|endFrame"Repository: tejpratap46/AndroidVideoMotion
Length of output: 189
🏁 Script executed:
# Look for Audio class definition in data or model files
find modules/motion-video-player -name "*.kt" | xargs rg "data class Audio|class Audio" -A 8Repository: tejpratap46/AndroidVideoMotion
Length of output: 58
🏁 Script executed:
# Check if there's any existing seekTo usage in the codebase
rg "seekTo" modules/motion-video-player/ -B 3 -A 3Repository: tejpratap46/AndroidVideoMotion
Length of output: 56
🏁 Script executed:
# Search for MotionVideoProducer to understand the structure better
rg "class MotionVideoProducer|interface MotionVideoProducer" modules/motion-video-player/ -A 15Repository: tejpratap46/AndroidVideoMotion
Length of output: 56
🏁 Script executed:
# Read the full MotionVideoPlayerCompose.kt file to see imports and understand the data structures
head -50 modules/motion-video-player/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayerCompose.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 2178
🏁 Script executed:
# Search for audio in all kotlin files more broadly
rg "motionAudio|delayFrame" modules/ -B 2 -A 2 --max-count 20Repository: tejpratap46/AndroidVideoMotion
Length of output: 23997
🏁 Script executed:
# Find all kotlin files in the motion-video-player module to understand structure
find modules/motion-video-player -name "*.kt" -type fRepository: tejpratap46/AndroidVideoMotion
Length of output: 189
🏁 Script executed:
# Check the old MotionVideoPlayer.kt (non-Compose) for audio handling and seekTo
grep -n "MediaPlayer\|seekTo\|startAudioIfNeeded" modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayer.kt -A 20 | head -100Repository: tejpratap46/AndroidVideoMotion
Length of output: 4321
🏁 Script executed:
# Specifically look at the audio player creation in the old version
sed -n '1,400p' modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/video/MotionVideoPlayer.kt | grep -A 30 "startAudioIfNeeded"Repository: tejpratap46/AndroidVideoMotion
Length of output: 2501
Seek audio to the current timeline position before every start() call.
When scrubbing the timeline or resuming playback within an active segment, audio players drift because they either restart from frame 0 (new players) or resume from their previous paused position (existing players). Derive the target seek position from (currentFrame - audio.delayFrame) * 1000 / fps in milliseconds and call seekTo(...) before starting.
Proposed fix
+ val safeFps = motionConfig.fps.coerceAtLeast(1)
if (shouldPlay) {
+ val targetMs =
+ (((currentFrame - audio.delayFrame).coerceAtLeast(0)) * 1000L / safeFps).toInt()
if (player == null) {
val mediaPlayer = MediaPlayer().apply {
setDataSource(audio.file.absolutePath)
prepare()
+ seekTo(targetMs)
start()
}
activePlayers[audio] = mediaPlayer
} else if (!player.isPlaying) {
+ player.seekTo(targetMs)
player.start()
}🤖 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 97 - 123, The audio players must be seeked to the current timeline
position before any start(): inside the LaunchedEffect handling
motionVideoProducer.motionAudio, compute the target position ms = ((currentFrame
- audio.delayFrame) * 1000) / fps (clamp to >=0) and call player.seekTo(ms)
before calling start() for both newly created MediaPlayer (created in the block
where activePlayers[audio] is null) and for existing paused players (when player
!= null and !player.isPlaying) so that playback aligns with the currentFrame;
ensure you use the existing activePlayers map, currentFrame, audio.delayFrame
and fps variables and call seekTo prior to start().
Comment on lines
+54
to
+59
| textColor?.let { | ||
| try { | ||
| this.setTextColor(it.toColorInt()) | ||
| } catch (e: Exception) { | ||
| // Fallback or log error | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/abstract/AbstractMotionTextView.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 3250
🏁 Script executed:
# Check if Timber is imported/used elsewhere in the project
rg "timber" --type kotlin -i | head -20Repository: tejpratap46/AndroidVideoMotion
Length of output: 3055
🏁 Script executed:
# Search for toColorInt usage patterns in the codebase
rg "toColorInt" --type kotlin -B 2 -A 2 | head -40Repository: tejpratap46/AndroidVideoMotion
Length of output: 5072
🌐 Web query:
androidx.core.graphics toColorInt IllegalArgumentException exception documentation
💡 Result:
In the AndroidX Core Graphics library, the String.toColorInt() extension method is a wrapper around the standard android.graphics.Color.parseColor(String) method [1]. It throws an IllegalArgumentException if the provided string cannot be parsed into a valid color [1][2]. Specifically, String.toColorInt() supports the following formats [1]: - Hexadecimal color strings in the formats #RRGGBB or #AARRGGBB [1]. - Standard color names (e.g., "red", "blue", "green", "black", "white", "gray", "cyan", "magenta", "yellow", "lightgray", "darkgray", "grey", "lightgrey", "darkgrey", "aqua", "fuchsia", "lime", "maroon", "navy", "olive", "purple", "silver", "teal") [1]. If the input string does not conform to these supported formats, or if it is malformed (such as an incorrect number of hex digits), the underlying Color.parseColor method will throw an IllegalArgumentException [1][2]. To handle this, it is recommended to wrap the call in a try-catch block to provide a fallback color or handle the error gracefully [2]. Note that while other methods in androidx.core.graphics.ColorUtils (such as compositeColors or setAlphaComponent) may also throw IllegalArgumentException when provided with invalid arguments (e.g., mismatched color models or out-of-range alpha values), toColorInt() specifically refers to the string parsing utility [3][4][5].
Citations:
- 1: https://android.googlesource.com/platform/frameworks/support/+/android-room-release/core/ktx/src/main/java/androidx/core/graphics/Color.kt
- 2: https://stackoverflow.com/questions/8225185/android-java-lang-illegalargumentexception-unknown-color
- 3: https://developer.android.com/reference/androidx/core/graphics/ColorUtils
- 4: https://android.googlesource.com/platform/frameworks/support/+/refs/heads/androidx-main/core/core/src/main/java/androidx/core/graphics/ColorUtils.java?autodive=0%2F%2F%2F%2F%2F%2F%2F%2F%2F%2F%2F
- 5: https://android.googlesource.com/platform/frameworks/support/+/8e2c3d4e6cccc493540605127883a6bfb795ac70%5E/compat/src/main/java/androidx/core/graphics/ColorUtils.java
Catch only IllegalArgumentException from toColorInt() and log the error.
Line 57 catches Exception broadly, which silently hides invalid color strings and unexpected runtime errors. The toColorInt() method specifically throws IllegalArgumentException for unparseable colors; catch only that exception type and log the fallback path using Timber (already used throughout the project).
Proposed fix
+import timber.log.Timber
@@
textColor?.let {
try {
this.setTextColor(it.toColorInt())
- } catch (e: Exception) {
- // Fallback or log error
+ } catch (e: IllegalArgumentException) {
+ Timber.w(e, "Invalid textColor '%s'. Using default text color.", 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/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/abstract/AbstractMotionTextView.kt`
around lines 54 - 59, Replace the broad Exception catch around the textColor
parsing with a specific IllegalArgumentException handler: in the textColor?.let
{ ... this.setTextColor(it.toColorInt()) ... } block catch only
IllegalArgumentException from toColorInt(), log the error via Timber (include
the invalid color string and a message about falling back to default) and keep
the fallback behavior; remove the generic catch so other runtime errors surface.
| MotionSdui.registerView(TransparentTextView::class.java.simpleName) { context, json -> | ||
| val props = json.parseMotionViewProps() | ||
| val text = json.get("text")?.asString ?: "" | ||
| val textSizeVariant = json.get("textSizeVariant")?.asString?.let { MotionTextVariant.valueOf(it) } |
Contributor
There was a problem hiding this comment.
Harden enum parsing for textSizeVariant to avoid payload-driven crashes.
MotionTextVariant.valueOf(...) throws on unknown values; a single bad SDUI value can abort view creation in Line [47], Line [72], Line [101], Line [129], and Line [157].
Proposed fix
- val textSizeVariant = json.get("textSizeVariant")?.asString?.let { MotionTextVariant.valueOf(it) }
+ val textSizeVariant =
+ json.get("textSizeVariant")?.asString?.let { value ->
+ runCatching { MotionTextVariant.valueOf(value) }.getOrNull()
+ }Also applies to: 72-72, 101-101, 129-129, 157-157
🤖 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/main/java/com/tejpratapsingh/motion/sdui/infra/MotionSduiInitializer.kt`
at line 47, The current parsing uses MotionTextVariant.valueOf(...) which throws
on unknown payload values and can crash view creation; replace these calls (the
ones setting textSizeVariant and the other similar enum parses) with a safe
lookup that returns null or a default if the incoming string is unrecognized
(e.g., use MotionTextVariant.values().firstOrNull { it.name == incomingString }
or wrap in runCatching and fallback), and update downstream code to handle the
nullable/default variant instead of allowing an exception to propagate from
MotionTextVariant.valueOf.
Comment on lines
9
to
+12
| const orientation = props.orientation || 'VERTICAL'; | ||
| const colors = props.colors || ['#000000', '#FFFFFF']; | ||
| const colorsStr = colors.join(', '); | ||
|
|
||
| let background = ''; | ||
| if (orientation === 'VERTICAL') { | ||
| background = `linear-gradient(to bottom, ${colorsStr})`; | ||
| } else if (orientation === 'HORIZONTAL') { | ||
| background = `linear-gradient(to right, ${colorsStr})`; | ||
| } else if (orientation === 'CIRCULAR') { | ||
| background = `radial-gradient(circle at center, ${colorsStr})`; | ||
| } | ||
| const gradientAngle = orientation === 'VERTICAL' ? '180deg' : '90deg'; | ||
| const background = `linear-gradient(${gradientAngle}, ${colors.join(', ')})`; |
Contributor
There was a problem hiding this comment.
Restore explicit CIRCULAR rendering behavior.
Line 11 currently maps every non-VERTICAL orientation to a 90deg linear gradient, which changes output for circular gradients.
Proposed fix
- const gradientAngle = orientation === 'VERTICAL' ? '180deg' : '90deg';
- const background = `linear-gradient(${gradientAngle}, ${colors.join(', ')})`;
+ const background =
+ orientation === 'CIRCULAR'
+ ? `radial-gradient(circle, ${colors.join(', ')})`
+ : `linear-gradient(${orientation === 'VERTICAL' ? '180deg' : '90deg'}, ${colors.join(', ')})`;📝 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
| const orientation = props.orientation || 'VERTICAL'; | |
| const colors = props.colors || ['#000000', '#FFFFFF']; | |
| const colorsStr = colors.join(', '); | |
| let background = ''; | |
| if (orientation === 'VERTICAL') { | |
| background = `linear-gradient(to bottom, ${colorsStr})`; | |
| } else if (orientation === 'HORIZONTAL') { | |
| background = `linear-gradient(to right, ${colorsStr})`; | |
| } else if (orientation === 'CIRCULAR') { | |
| background = `radial-gradient(circle at center, ${colorsStr})`; | |
| } | |
| const gradientAngle = orientation === 'VERTICAL' ? '180deg' : '90deg'; | |
| const background = `linear-gradient(${gradientAngle}, ${colors.join(', ')})`; | |
| const orientation = props.orientation || 'VERTICAL'; | |
| const background = | |
| orientation === 'CIRCULAR' | |
| ? `radial-gradient(circle, ${colors.join(', ')})` | |
| : `linear-gradient(${orientation === 'VERTICAL' ? '180deg' : '90deg'}, ${colors.join(', ')})`; |
🤖 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 `@web/web-sdui/src/views/GradientView.tsx` around lines 9 - 12, The current
orientation handling in GradientView.tsx treats any non-'VERTICAL' orientation
as a 90deg linear gradient, breaking explicit 'CIRCULAR' behavior; update the
logic around the orientation/gradientAngle/background (the variables named
orientation, gradientAngle, and background) to branch explicitly: keep
'VERTICAL' => linear 180deg, handle 'HORIZONTAL' (or other linear orientations)
=> linear 90deg, and implement 'CIRCULAR' to produce a radial gradient (e.g.,
use a radial-gradient with the colors array) instead of a linear-gradient so
circular gradients render correctly.
* templates: introduce `MotionTemplate` DSL with structured `parameters` and `content` blocks * templates: add DSL extension functions for text, image, video, background, and audio components * templates: implement `TemplateSerialization` to support JSON import/export and data application * templates: add `TemplateData` and `TemplateParameter` models for type-safe data handling * motionlib: update `MotionVideoProducer` to support dynamic audio addition and gravity-based positioning * templates: update documentation with complex DSL and JSON template examples * templates: add unit and instrumentation tests for DSL structure and serialization logic
tejpratap46
force-pushed
the
feat/implement-templates
branch
from
2463304 to
d9df255
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.
MotionTemplateandMotionTemplateApplierfor JSON placeholder replacement{{REPLICATE}}markerMotionTemplateViewGeneratorto bridge templates withMotionViewcreationRef: [Templates] Implement Templates for Lyrics #4
Summary by CodeRabbit