Support Kotlin Multiplatform#983
Open
skydoves wants to merge 3 commits into
Open
Conversation
WalkthroughThis PR adds a new Compose Multiplatform "balloon-compose-multiplatform" module implementing a platform-agnostic popup/balloon system with Compose state, shape/path geometry, animations, and a builder DSL. It wires Android, Desktop, iOS, and Wasm sample apps, adds API baselines, migration docs, and tests. ChangesCompose Multiplatform Balloon introduction
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant BalloonState
participant BalloonPopupKt.Balloon
participant BalloonPopupPositionProvider
participant BalloonContent
Caller->>BalloonState: show/showAtCenter(...)
BalloonPopupKt.Balloon->>BalloonPopupPositionProvider: calculatePosition(...)
BalloonPopupPositionProvider->>BalloonState: update resolvedArrowOrientation/resolvedArrowRatio
BalloonPopupKt.Balloon->>BalloonContent: render popup content with transitions
Caller->>BalloonState: dismiss()
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
androidApp/src/androidMain/AndroidManifest.xml (1)
19-23: ⚡ Quick winDisable platform backups for the demo app.
android:allowBackup="true"enables app data backup/restore by default. For a demo app, hardening tofalseavoids accidental persistence/leakage of future local data.Suggested manifest hardening
<application - android:allowBackup="true" + android:allowBackup="false" android:label="Balloon KMP Demo" android:supportsRtl="true" android:theme="`@android`:style/Theme.Material.Light.NoActionBar">🤖 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 `@androidApp/src/androidMain/AndroidManifest.xml` around lines 19 - 23, The android:allowBackup attribute in the application tag is currently set to true, which enables automatic app data backup and restore functionality. For a demo application, this should be disabled to prevent accidental persistence or leakage of local data. Change the android:allowBackup attribute value from true to false in the application element to harden the manifest configuration.balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/Balloon.kt (1)
191-191: ⚡ Quick winMove
@BalloonDslonto the receiver type.Annotating
rememberBalloonBuilderdoes not markBalloon.Builderas a DSL receiver, so nested builder scopes stay unrestricted. Put the marker onBalloon.Builder(or on the function type) if you want the DSL guard to apply.Suggested fix
- public class Builder { + `@BalloonDsl` + public class Builder { private var cornerRadius: Dp = 5.dp ... } } /** DSL marker for the fluent [Balloon.Builder] receiver lambda. */ `@DslMarker` public annotation class BalloonDsl `@Composable` -@BalloonDsl public fun rememberBalloonBuilder( key: Any? = null, block: Balloon.Builder.() -> Unit, ): BalloonStyle = remember(key) { Balloon.Builder().apply(block).build() }Also applies to: 382-387
🤖 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 `@balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/Balloon.kt` at line 191, The `@BalloonDsl` annotation needs to be moved from the rememberBalloonBuilder function to the actual receiver type to properly enforce DSL scope restrictions. At line 191, add the `@BalloonDsl` annotation directly to the public class Builder declaration for Balloon.Builder. Apply the same fix to the other affected Builder class at lines 382-387 by adding `@BalloonDsl` to its class declaration as well. This ensures the DSL guard applies to nested builder scopes in both locations.
🤖 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 `@balloon-compose-multiplatform/MIGRATION.md`:
- Around line 150-151: The migration documentation for
setOnBalloonDismissListener is currently directing users to observe
BalloonState.isVisible or use LaunchedEffect, which is imprecise and misses the
intended transition semantics. Update the migration table entry for
setOnBalloonDismissListener to instead map it directly to
BalloonState.onDismiss, since that callback already provides the same
visible→hidden semantics as a one-shot handler and is the more direct and
accurate migration path.
- Around line 139-147: The Markdown table contains raw pipe characters (`|`)
within cell content that are breaking the table structure. Escape these pipes by
replacing them with `\|`. Specifically, in entries like
`setTextTypeface(Int|Typeface)` and `setLayout(View|`@LayoutRes`
Int|ViewBinding)`, add backslashes before each pipe character that appears
within the type signatures and method names to properly escape them in the
Markdown table format.
In
`@balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/Balloon.kt`:
- Around line 315-318: The setAnimationDurationMillis method accepts animation
duration values without validation, but these values are passed to tween(...)
which requires non-negative durations per its `@IntRange`(from = 0) constraint.
Clamp the input value to zero if it's negative before assigning it to
animationDurationMillis, using the same validation pattern already implemented
in setAutoDismissDuration to prevent runtime failures when negative durations
are provided.
In
`@balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonPopup.kt`:
- Around line 250-256: The arrow orientation mapping for side-aligned balloons
(START and END) incorrectly applies RTL logic that causes the arrow to render on
the wrong side initially. Replace the RTL conditional logic in the
BalloonAlign.START and BalloonAlign.END branches with their logical opposite
edges: BalloonAlign.START should always map to ArrowOrientation.END, and
BalloonAlign.END should always map to ArrowOrientation.START, regardless of the
isRtl value. This ensures the arrow is positioned on the correct logical
opposite edge from the start, preventing incorrect animation pivot and rendering
in the first frame.
In
`@balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonShapeBuilder.kt`:
- Around line 194-261: The arrow overlay triangle is being positioned using
different clamping logic than the balloon body notch in buildBalloonPath,
causing misalignment when the arrow has a different color than the background
near rounded corners. In buildArrowTrianglePath, the center position
calculations (centerX for TOP and BOTTOM cases, centerY for LEFT and RIGHT
cases) are currently clamping only against halfArrow + halfStrokePx bounds. You
need to identify how buildBalloonPath clamps the center position (which includes
radius + halfArrow constraints) and apply that same clamping logic to all four
when branches (TOP, BOTTOM, LEFT, RIGHT) in buildArrowTrianglePath so the arrow
overlay aligns with the notch.
In
`@balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonState.kt`:
- Around line 164-174: The show() function in BalloonState needs to reset the
resolved arrow cache properties when displaying a new balloon. Currently,
resolvedArrowOrientation and resolvedArrowRatio persist across dismiss/show
cycles, causing stale arrow placement data to be used in the first frame after
reopening. Add lines in the show() function to reset resolvedArrowOrientation
and resolvedArrowRatio to their default/initial values, similar to how
centerAlign is reset to null, to ensure fresh arrow placement computation on
each show cycle. Also check lines 200-210 for any related show method or
overload that requires the same fix.
In
`@balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonStyle.kt`:
- Around line 66-84: The BalloonStyle data class constructor does not validate
its documented invariants, allowing invalid values to propagate downstream. Add
an init block to the BalloonStyle data class that validates: arrowPosition must
be in the range 0f to 1f (inclusive), animationDurationMillis must be
non-negative, and autoDismissMillis must be non-negative. Each validation should
throw an IllegalArgumentException with a descriptive message if the constraint
is violated, ensuring that only valid BalloonStyle instances can be constructed.
In `@desktopApp/build.gradle.kts`:
- Around line 48-49: The packageVersion property in desktopApp/build.gradle.kts
is hardcoded to "1.0.0", which can cause version drift from the project's actual
release version. Replace the hardcoded packageVersion value with a reference to
the project's shared version source (such as the project.version property or a
version variable defined in the gradle configuration). This ensures the desktop
distribution version stays synchronized with the project's canonical release
version definition.
In `@iosApp/iosApp.xcodeproj/project.pbxproj`:
- Line 334: The PRODUCT_BUNDLE_IDENTIFIER is incorrectly composed by
concatenating BUNDLE_ID with TEAM_ID, which violates iOS provisioning
conventions and can break signing. In iosApp/iosApp.xcodeproj/project.pbxproj at
line 334 and line 361, change PRODUCT_BUNDLE_IDENTIFIER from
"${BUNDLE_ID}${TEAM_ID}" to just "${BUNDLE_ID}" (removing the TEAM_ID
concatenation). The TEAM_ID should only be used in the DEVELOPMENT_TEAM setting,
not in the bundle identifier itself.
In
`@iosApp/iosApp.xcodeproj/xcuserdata/jaewoong.xcuserdatad/xcschemes/iosApp.xcscheme`:
- Around line 1-32: The iosApp.xcscheme file is located under xcuserdata, which
contains user-specific Xcode configuration and should not be committed to
version control. Remove this file from the pull request by excluding the entire
xcuserdata directory from git tracking, and ensure xcuserdata is listed in the
.gitignore file. If a shared Xcode scheme is needed across team members, move
the scheme file to xcshareddata/xcschemes instead, which is the appropriate
location for shared build configurations.
In `@iosApp/iosApp/Assets.xcassets/AppIcon.appiconset/Contents.json`:
- Around line 2-12: The AppIcon.appiconset Contents.json file defines image
entries without corresponding filename references. For the universal platform
iOS image entry at 1024x1024 size, add a "filename" key that points to the
actual PNG image file (e.g., "AppIcon-1024.png"), then ensure the actual image
file with that name is present in the AppIcon.appiconset directory. If multiple
icon sizes are needed for iOS, add additional image entries to the Contents.json
array with appropriate sizes, idioms, and filenames, along with their
corresponding image files in the directory, so that Xcode can properly bundle
the app icon during the build process.
In `@wasmApp/src/wasmJsMain/resources/index.html`:
- Line 8: The script tag loading composeApp.js in the index.html file does not
have the defer attribute, causing it to execute before the body element is
available. This causes a crash in Main.kt when attempting to access
document.body. Add the defer attribute to the script tag that loads
composeApp.js to defer its execution until after the DOM is fully loaded and the
body element is available.
---
Nitpick comments:
In `@androidApp/src/androidMain/AndroidManifest.xml`:
- Around line 19-23: The android:allowBackup attribute in the application tag is
currently set to true, which enables automatic app data backup and restore
functionality. For a demo application, this should be disabled to prevent
accidental persistence or leakage of local data. Change the android:allowBackup
attribute value from true to false in the application element to harden the
manifest configuration.
In
`@balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/Balloon.kt`:
- Line 191: The `@BalloonDsl` annotation needs to be moved from the
rememberBalloonBuilder function to the actual receiver type to properly enforce
DSL scope restrictions. At line 191, add the `@BalloonDsl` annotation directly to
the public class Builder declaration for Balloon.Builder. Apply the same fix to
the other affected Builder class at lines 382-387 by adding `@BalloonDsl` to its
class declaration as well. This ensures the DSL guard applies to nested builder
scopes in both locations.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3af4c74-5676-44b8-a8ae-de5f716bd32a
📒 Files selected for processing (47)
.gitignoreandroidApp/build.gradle.ktsandroidApp/src/androidMain/AndroidManifest.xmlandroidApp/src/androidMain/kotlin/com/skydoves/balloon/kmpdemo/KmpBalloonActivity.ktballoon-compose-multiplatform/MIGRATION.mdballoon-compose-multiplatform/api/android/balloon-compose-multiplatform.apiballoon-compose-multiplatform/api/balloon-compose-multiplatform.klib.apiballoon-compose-multiplatform/api/desktop/balloon-compose-multiplatform.apiballoon-compose-multiplatform/build.gradle.ktsballoon-compose-multiplatform/src/androidMain/AndroidManifest.xmlballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/ArrowOrientation.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/ArrowPositionRules.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/Balloon.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonAlign.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonAnimation.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonCenterAlign.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonPopup.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonShape.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonShapeBuilder.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonState.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonStyle.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonTransitions.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/TransformOrigins.ktballoon-compose-multiplatform/src/commonTest/kotlin/com/skydoves/balloon/compose/multiplatform/PositionProviderTest.ktbuild.gradle.ktsdesktopApp/build.gradle.ktsdesktopApp/src/desktopMain/kotlin/Main.ktgradle/libs.versions.tomliosApp/Configuration/Config.xcconfigiosApp/iosApp.xcodeproj/project.pbxprojiosApp/iosApp.xcodeproj/xcuserdata/jaewoong.xcuserdatad/xcschemes/iosApp.xcschemeiosApp/iosApp.xcodeproj/xcuserdata/jaewoong.xcuserdatad/xcschemes/xcschememanagement.plistiosApp/iosApp/Assets.xcassets/AccentColor.colorset/Contents.jsoniosApp/iosApp/Assets.xcassets/AppIcon.appiconset/Contents.jsoniosApp/iosApp/Assets.xcassets/Contents.jsoniosApp/iosApp/ContentView.swiftiosApp/iosApp/Info.plistiosApp/iosApp/Preview Content/Preview Assets.xcassets/Contents.jsoniosApp/iosApp/iOSApp.swiftsamples-shared/build.gradle.ktssamples-shared/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/sample/BalloonDemoScreen.ktsamples-shared/src/iosMain/kotlin/com/skydoves/balloon/compose/multiplatform/sample/MainViewController.ktsettings.gradle.ktswasmApp/build.gradle.ktswasmApp/src/wasmJsMain/kotlin/Main.ktwasmApp/src/wasmJsMain/resources/index.htmlwasmApp/src/wasmJsMain/resources/style.css
Comment on lines
+139
to
+147
| | `setTextTypeface(Int|Typeface)` | _not supported in KMP_ | `Typeface` is Android-only; pass a `FontFamily` to the `Text(...)` directly. | | ||
| | `setTextLineSpacing(...)` / `setTextLetterSpacing(...)` (+ `*Resource`) | _not supported in KMP_ | Set on the `Text(...)` directly. | | ||
| | `setIncludeFontPadding(...)` | _not supported in KMP_ | Android-`TextView`-specific. | | ||
| | `setTextGravity(Int)` | _not supported in KMP_ | Use `Text(textAlign = ...)`. | | ||
| | `setTextForm(TextForm)` | _not supported in KMP_ | View-only abstraction. | | ||
| | `setIcon*` (all variants) | _not a builder concern_ | Compose icons go inside the slot — `Row { Icon(...); Text(...) }`. | | ||
| | `setAlpha(Float)` | _not supported in KMP_ | Wrap the slot in `Modifier.alpha(...)` if needed. | | ||
| | `setElevation(Int)` (+ `*Resource`) | _not supported in KMP_ | Wrap the slot in `Modifier.shadow(...)`. | | ||
| | `setLayout(View|@LayoutRes Int|ViewBinding)` | _not a builder concern_ | The whole point of the Compose slot. | |
There was a problem hiding this comment.
Escape the | characters in these table entries.
Raw pipes inside the type names are breaking the Markdown table structure, which is why markdownlint flags these rows and the rendered docs can lose part of the signature.
Fix
-| `setTextTypeface(Int|Typeface)` | _not supported in KMP_ | `Typeface` is Android-only; pass a `FontFamily` to the `Text(...)` directly. |
+| `setTextTypeface(Int\|Typeface)` | _not supported in KMP_ | `Typeface` is Android-only; pass a `FontFamily` to the `Text(...)` directly. |
...
-| `setLayout(View|`@LayoutRes` Int|ViewBinding)` | _not a builder concern_ | The whole point of the Compose slot. |
+| `setLayout(View\|`@LayoutRes` Int\|ViewBinding)` | _not a builder concern_ | The whole point of the Compose slot. |🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 139-139: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 147-147: Table column count
Expected: 3; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 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 `@balloon-compose-multiplatform/MIGRATION.md` around lines 139 - 147, The
Markdown table contains raw pipe characters (`|`) within cell content that are
breaking the table structure. Escape these pipes by replacing them with `\|`.
Specifically, in entries like `setTextTypeface(Int|Typeface)` and
`setLayout(View|`@LayoutRes` Int|ViewBinding)`, add backslashes before each pipe
character that appears within the type signatures and method names to properly
escape them in the Markdown table format.
Source: Linters/SAST tools
Comment on lines
+150
to
+151
| | `setOnBalloonDismissListener(...)` | Observe `BalloonState.isVisible` | Or compose a `LaunchedEffect` keyed on the state. | | ||
| | `setOnBalloonInitializedListener(...)` | _not needed in KMP_ | Compose callbacks (`onGloballyPositioned`) fire naturally. | |
There was a problem hiding this comment.
Map the legacy dismiss listener to BalloonState.onDismiss.
BalloonState already exposes a one-shot onDismiss callback with the same visible→hidden semantics, so telling migrators to observe isVisible or wire a LaunchedEffect is less precise and can miss the intended transition.
Fix
-| `setOnBalloonDismissListener(...)` | Observe `BalloonState.isVisible` | Or compose a `LaunchedEffect` keyed on the state. |
+| `setOnBalloonDismissListener(...)` | `state.onDismiss = { ... }` | Direct KMP equivalent; fires once per visible→hidden transition. |📝 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
| | `setOnBalloonDismissListener(...)` | Observe `BalloonState.isVisible` | Or compose a `LaunchedEffect` keyed on the state. | | |
| | `setOnBalloonInitializedListener(...)` | _not needed in KMP_ | Compose callbacks (`onGloballyPositioned`) fire naturally. | | |
| | `setOnBalloonDismissListener(...)` | `state.onDismiss = { ... }` | Direct KMP equivalent; fires once per visible→hidden transition. | | |
| | `setOnBalloonInitializedListener(...)` | _not needed in KMP_ | Compose callbacks (`onGloballyPositioned`) fire naturally. | |
🤖 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 `@balloon-compose-multiplatform/MIGRATION.md` around lines 150 - 151, The
migration documentation for setOnBalloonDismissListener is currently directing
users to observe BalloonState.isVisible or use LaunchedEffect, which is
imprecise and misses the intended transition semantics. Update the migration
table entry for setOnBalloonDismissListener to instead map it directly to
BalloonState.onDismiss, since that callback already provides the same
visible→hidden semantics as a one-shot handler and is the more direct and
accurate migration path.
Comment on lines
+315
to
+318
| /** Sets the duration of the [BalloonAnimation] in milliseconds. */ | ||
| public fun setAnimationDurationMillis(value: Int): Builder = apply { | ||
| animationDurationMillis = value | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the Balloon.kt file and examine the setAnimationDurationMillis method
find . -name "Balloon.kt" -type f | head -20Repository: skydoves/Balloon
Length of output: 300
🏁 Script executed:
# Find and examine BalloonTransitions.kt to see how animationDurationMillis is used
find . -name "*Transitions*" -o -name "*transitions*" | grep -i kotlinRepository: skydoves/Balloon
Length of output: 179
🏁 Script executed:
# Search for setAutoDismissDuration to compare validation approach
rg "setAutoDismissDuration" -A 5 -B 2Repository: skydoves/Balloon
Length of output: 17517
🏁 Script executed:
# Read the target file to see setAnimationDurationMillis and the animationDurationMillis field
cat -n balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/Balloon.kt | sed -n '1,350p'Repository: skydoves/Balloon
Length of output: 16898
🏁 Script executed:
# Read BalloonTransitions.kt to see how animationDurationMillis is used
cat -n balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonTransitions.ktRepository: skydoves/Balloon
Length of output: 5651
🏁 Script executed:
# Check where animationDurationMillis from the builder is used
rg "animationDurationMillis" balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/ -A 3 -B 3Repository: skydoves/Balloon
Length of output: 7361
🏁 Script executed:
# Check the rest of the Balloon.kt file to see the build() method
cat -n balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/Balloon.kt | sed -n '340,380p'Repository: skydoves/Balloon
Length of output: 1814
🌐 Web query:
Compose animation tween negative duration behavior runtime error
💡 Result:
In Jetpack Compose, animation specifications that rely on duration—such as tween, keyframes, and repeatable—require a non-negative duration [1][2][3]. The durationMillis property in TweenSpec and related DurationBasedAnimationSpec implementations is explicitly annotated with @IntRange(from = 0L) [2][3]. Providing a negative value to these constructors will violate this range constraint. Depending on the environment (e.g., if validation is enabled via annotations or during internal framework processing), this may result in an IllegalArgumentException or undefined animation behavior, such as the animation failing to trigger or completing instantly [2][3]. To ensure stable behavior, always provide a duration of 0 or greater [2][3]. If you do not require a specific duration, using the default DefaultDurationMillis (which is 300ms) or a positive integer is the standard practice [2][3][4]. If an animation must appear to happen instantly, snap() is the recommended alternative rather than using a zero or negative duration [5].
Citations:
- 1: https://developer.android.com/reference/kotlin/androidx/compose/animation/core/DurationBasedAnimationSpec
- 2: https://android.googlesource.com/platform/frameworks/support/+/3f48e8e1371cf70762bc8f9eed7ec43631c077fd/compose/animation/animation-core/src/commonMain/kotlin/androidx/compose/animation/core/AnimationSpec.kt
- 3: https://android.googlesource.com/platform/frameworks/support/+/a18f72ab3de68971fb30d894d41f4441aa09fd4f/compose/animation/animation-core/src/commonMain/kotlin/androidx/compose/animation/core/AnimationSpec.kt
- 4: https://developer.android.com/reference/kotlin/androidx/compose/animation/core/TweenSpec
- 5: https://developer.android.com/develop/ui/compose/animation/quick-guide
Validate animation durations to prevent runtime failures.
setAnimationDurationMillis stores raw values without validation, but they're passed directly to tween(...) in balloonEnterTransition / balloonExitTransition. Negative durations violate Compose's @IntRange(from = 0) constraint on animation specs and will fail at runtime. Clamp values to zero, matching the validation pattern already used in setAutoDismissDuration.
Suggested fix
public fun setAnimationDurationMillis(value: Int): Builder = apply {
- animationDurationMillis = value
+ animationDurationMillis = value.coerceAtLeast(0)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** Sets the duration of the [BalloonAnimation] in milliseconds. */ | |
| public fun setAnimationDurationMillis(value: Int): Builder = apply { | |
| animationDurationMillis = value | |
| } | |
| /** Sets the duration of the [BalloonAnimation] in milliseconds. */ | |
| public fun setAnimationDurationMillis(value: Int): Builder = apply { | |
| animationDurationMillis = value.coerceAtLeast(0) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/Balloon.kt`
around lines 315 - 318, The setAnimationDurationMillis method accepts animation
duration values without validation, but these values are passed to tween(...)
which requires non-negative durations per its `@IntRange`(from = 0) constraint.
Clamp the input value to zero if it's negative before assigning it to
animationDurationMillis, using the same validation pattern already implemented
in setAutoDismissDuration to prevent runtime failures when negative durations
are provided.
Comment on lines
+250
to
+256
| return when (align) { | ||
| BalloonAlign.TOP -> ArrowOrientation.BOTTOM | ||
| BalloonAlign.BOTTOM -> ArrowOrientation.TOP | ||
| // Balloon on the leading side -> arrow points back to the trailing side. | ||
| BalloonAlign.START -> if (isRtl) ArrowOrientation.START else ArrowOrientation.END | ||
| // Balloon on the trailing side -> arrow points back to the leading side. | ||
| BalloonAlign.END -> if (isRtl) ArrowOrientation.END else ArrowOrientation.START |
There was a problem hiding this comment.
Use the logical opposite edge in the fallback START/END mapping.
For side-aligned balloons, the arrow should sit on the logical opposite edge regardless of layout direction: START -> END, END -> START. The current RTL branches invert that, so the first frame in RTL can draw the arrow and animation pivot on the wrong side before the position provider corrects it.
Suggested fix
return when (align) {
BalloonAlign.TOP -> ArrowOrientation.BOTTOM
BalloonAlign.BOTTOM -> ArrowOrientation.TOP
- BalloonAlign.START -> if (isRtl) ArrowOrientation.START else ArrowOrientation.END
- BalloonAlign.END -> if (isRtl) ArrowOrientation.END else ArrowOrientation.START
+ BalloonAlign.START -> ArrowOrientation.END
+ BalloonAlign.END -> ArrowOrientation.START
BalloonAlign.CENTER -> ArrowOrientation.BOTTOM
}📝 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
| return when (align) { | |
| BalloonAlign.TOP -> ArrowOrientation.BOTTOM | |
| BalloonAlign.BOTTOM -> ArrowOrientation.TOP | |
| // Balloon on the leading side -> arrow points back to the trailing side. | |
| BalloonAlign.START -> if (isRtl) ArrowOrientation.START else ArrowOrientation.END | |
| // Balloon on the trailing side -> arrow points back to the leading side. | |
| BalloonAlign.END -> if (isRtl) ArrowOrientation.END else ArrowOrientation.START | |
| return when (align) { | |
| BalloonAlign.TOP -> ArrowOrientation.BOTTOM | |
| BalloonAlign.BOTTOM -> ArrowOrientation.TOP | |
| // Balloon on the leading side -> arrow points back to the trailing side. | |
| BalloonAlign.START -> ArrowOrientation.END | |
| // Balloon on the trailing side -> arrow points back to the leading side. | |
| BalloonAlign.END -> ArrowOrientation.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
`@balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonPopup.kt`
around lines 250 - 256, The arrow orientation mapping for side-aligned balloons
(START and END) incorrectly applies RTL logic that causes the arrow to render on
the wrong side initially. Replace the RTL conditional logic in the
BalloonAlign.START and BalloonAlign.END branches with their logical opposite
edges: BalloonAlign.START should always map to ArrowOrientation.END, and
BalloonAlign.END should always map to ArrowOrientation.START, regardless of the
isRtl value. This ensures the arrow is positioned on the correct logical
opposite edge from the start, preventing incorrect animation pivot and rendering
in the first frame.
Comment on lines
+48
to
+49
| packageName = "BalloonComposeMultiplatformDemo" | ||
| packageVersion = "1.0.0" |
There was a problem hiding this comment.
Align desktop distribution version with shared release version source.
Line 49 hardcodes packageVersion = "1.0.0", which can drift from the project’s actual release versioning and produce mismatched desktop artifacts.
Suggested fix
+import com.skydoves.balloon.Configuration
import org.jetbrains.compose.desktop.application.dsl.TargetFormat
@@
- packageVersion = "1.0.0"
+ packageVersion = Configuration.versionName📝 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
| packageName = "BalloonComposeMultiplatformDemo" | |
| packageVersion = "1.0.0" | |
| import com.skydoves.balloon.Configuration | |
| import org.jetbrains.compose.desktop.application.dsl.TargetFormat | |
| packageName = "BalloonComposeMultiplatformDemo" | |
| packageVersion = Configuration.versionName |
🤖 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 `@desktopApp/build.gradle.kts` around lines 48 - 49, The packageVersion
property in desktopApp/build.gradle.kts is hardcoded to "1.0.0", which can cause
version drift from the project's actual release version. Replace the hardcoded
packageVersion value with a reference to the project's shared version source
(such as the project.version property or a version variable defined in the
gradle configuration). This ensures the desktop distribution version stays
synchronized with the project's canonical release version definition.
| "$(inherited)", | ||
| "@executable_path/Frameworks", | ||
| ); | ||
| PRODUCT_BUNDLE_IDENTIFIER = "${BUNDLE_ID}${TEAM_ID}"; |
There was a problem hiding this comment.
Use a stable bundle identifier; do not append team ID.
At Line 334 and Line 361, PRODUCT_BUNDLE_IDENTIFIER is composed as "${BUNDLE_ID}${TEAM_ID}". TEAM_ID belongs in DEVELOPMENT_TEAM, not in the bundle ID. This can produce unintended identifiers and break provisioning/signing expectations.
Suggested fix
- PRODUCT_BUNDLE_IDENTIFIER = "${BUNDLE_ID}${TEAM_ID}";
+ PRODUCT_BUNDLE_IDENTIFIER = "${BUNDLE_ID}";Also applies to: 361-361
🤖 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 `@iosApp/iosApp.xcodeproj/project.pbxproj` at line 334, The
PRODUCT_BUNDLE_IDENTIFIER is incorrectly composed by concatenating BUNDLE_ID
with TEAM_ID, which violates iOS provisioning conventions and can break signing.
In iosApp/iosApp.xcodeproj/project.pbxproj at line 334 and line 361, change
PRODUCT_BUNDLE_IDENTIFIER from "${BUNDLE_ID}${TEAM_ID}" to just "${BUNDLE_ID}"
(removing the TEAM_ID concatenation). The TEAM_ID should only be used in the
DEVELOPMENT_TEAM setting, not in the bundle identifier itself.
Comment on lines
+1
to
+32
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <Scheme | ||
| version = "1.3"> | ||
| <BuildAction> | ||
| <BuildActionEntries> | ||
| <BuildActionEntry | ||
| buildForRunning = "YES"> | ||
| <BuildableReference | ||
| BuildableIdentifier = "primary" | ||
| BlueprintIdentifier = "7555FF7A242A565900829871" | ||
| BuildableName = "BalloonCMPDemo.app" | ||
| BlueprintName = "iosApp" | ||
| ReferencedContainer = "container:iosApp.xcodeproj"> | ||
| </BuildableReference> | ||
| </BuildActionEntry> | ||
| </BuildActionEntries> | ||
| </BuildAction> | ||
| <LaunchAction | ||
| useCustomWorkingDirectory = "NO" | ||
| buildConfiguration = "Debug" | ||
| allowLocationSimulation = "YES"> | ||
| <BuildableProductRunnable> | ||
| <BuildableReference | ||
| BuildableIdentifier = "primary" | ||
| BlueprintIdentifier = "7555FF7A242A565900829871" | ||
| BuildableName = "BalloonCMPDemo.app" | ||
| BlueprintName = "iosApp" | ||
| ReferencedContainer = "container:iosApp.xcodeproj"> | ||
| </BuildableReference> | ||
| </BuildableProductRunnable> | ||
| </LaunchAction> | ||
| </Scheme> |
There was a problem hiding this comment.
Do not commit user-specific Xcode scheme data.
This file is under xcuserdata, which is local-user state and should stay untracked. Keep shared schemes under xcshareddata/xcschemes if needed.
🤖 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
`@iosApp/iosApp.xcodeproj/xcuserdata/jaewoong.xcuserdatad/xcschemes/iosApp.xcscheme`
around lines 1 - 32, The iosApp.xcscheme file is located under xcuserdata, which
contains user-specific Xcode configuration and should not be committed to
version control. Remove this file from the pull request by excluding the entire
xcuserdata directory from git tracking, and ensure xcuserdata is listed in the
.gitignore file. If a shared Xcode scheme is needed across team members, move
the scheme file to xcshareddata/xcschemes instead, which is the appropriate
location for shared build configurations.
Comment on lines
+2
to
+12
| "images" : [ | ||
| { | ||
| "idiom" : "universal", | ||
| "platform" : "ios", | ||
| "size" : "1024x1024" | ||
| } | ||
| ], | ||
| "info" : { | ||
| "author" : "xcode", | ||
| "version" : 1 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a . iosApp/iosApp/Assets.xcassets/AppIcon.appiconset
rg -n '"filename"\s*:' iosApp/iosApp/Assets.xcassets/AppIcon.appiconset/Contents.jsonRepository: skydoves/Balloon
Length of output: 142
Add image file(s) to the AppIcon asset set.
The AppIcon.appiconset contains only a metadata file with no filename entry and no accompanying image files. Xcode cannot bundle an app icon without at least one valid image reference. Either add the icon PNG file(s) to this directory and reference them in Contents.json with a "filename" key, or verify the icon is being sourced from elsewhere in the build process.
🤖 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 `@iosApp/iosApp/Assets.xcassets/AppIcon.appiconset/Contents.json` around lines
2 - 12, The AppIcon.appiconset Contents.json file defines image entries without
corresponding filename references. For the universal platform iOS image entry at
1024x1024 size, add a "filename" key that points to the actual PNG image file
(e.g., "AppIcon-1024.png"), then ensure the actual image file with that name is
present in the AppIcon.appiconset directory. If multiple icon sizes are needed
for iOS, add additional image entries to the Contents.json array with
appropriate sizes, idioms, and filenames, along with their corresponding image
files in the directory, so that Xcode can properly bundle the app icon during
the build process.
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <title>Balloon Compose Multiplatform - Wasm Demo</title> | ||
| <link type="text/css" rel="stylesheet" href="style.css"> | ||
| <script type="application/javascript" src="composeApp.js"></script> |
There was a problem hiding this comment.
Defer Wasm bundle loading to avoid early startup crash.
At Line 8, loading composeApp.js in <head> without defer can run before <body> is available, which can crash wasmApp/src/wasmJsMain/kotlin/Main.kt at Line 24 (document.body!!).
Suggested fix
- <script type="application/javascript" src="composeApp.js"></script>
+ <script type="application/javascript" src="composeApp.js" defer></script>🤖 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 `@wasmApp/src/wasmJsMain/resources/index.html` at line 8, The script tag
loading composeApp.js in the index.html file does not have the defer attribute,
causing it to execute before the body element is available. This causes a crash
in Main.kt when attempting to access document.body. Add the defer attribute to
the script tag that loads composeApp.js to defer its execution until after the
DOM is fully loaded and the body element is available.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonHost.kt`:
- Around line 155-159: The anchor bounds conversion in BalloonHost’s
onGloballyPositioned callback is using a nonexistent Rect conversion, so update
the coordinates.boundsInWindow() handling to use roundToIntRect() instead of
toIntRect(). Make this change in the code path that assigns entry.anchorBounds
so the bounds conversion compiles correctly.
In
`@balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonShapeBuilder.kt`:
- Line 252: The center calculation in BalloonShapeBuilder should handle the
degenerate size case before calling coerceIn, because the current bounds can
become invalid when fullExtent is smaller than twice halfArrow. Update the logic
around the center computation to guard for this condition first and either clamp
the min/max bounds safely or fall back to the midpoint, so small balloons with
wide arrows don’t throw. Keep the fix localized to the center/arrow placement
logic in BalloonShapeBuilder.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1fb3582c-e6e8-43f6-b36f-7b85952378c3
📒 Files selected for processing (16)
balloon-compose-multiplatform/api/android/balloon-compose-multiplatform.apiballoon-compose-multiplatform/api/balloon-compose-multiplatform.klib.apiballoon-compose-multiplatform/api/desktop/balloon-compose-multiplatform.apiballoon-compose-multiplatform/src/androidMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonPopupProperties.android.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/Balloon.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonHost.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonPopup.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonPopupProperties.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonShape.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonShapeBuilder.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonState.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonStyle.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonTransitions.ktballoon-compose-multiplatform/src/commonTest/kotlin/com/skydoves/balloon/compose/multiplatform/PositionProviderTest.ktballoon-compose-multiplatform/src/skiaMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonPopupProperties.skia.ktsamples-shared/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/sample/ModifierBalloonSample.kt
✅ Files skipped from review due to trivial changes (1)
- balloon-compose-multiplatform/api/android/balloon-compose-multiplatform.api
🚧 Files skipped from review as they are similar to previous changes (4)
- balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonStyle.kt
- balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/Balloon.kt
- balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonPopup.kt
- balloon-compose-multiplatform/src/commonTest/kotlin/com/skydoves/balloon/compose/multiplatform/PositionProviderTest.kt
Comment on lines
+155
to
+159
| return this.onGloballyPositioned { coordinates -> | ||
| val bounds = coordinates.boundsInWindow().toIntRect() | ||
| if (entry.anchorBounds != bounds) { | ||
| entry.anchorBounds = bounds | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does androidx.compose.ui.geometry.Rect have a toIntRect() extension function, or only roundToIntRect()?
💡 Result:
androidx.compose.ui.geometry.Rect does not have a toIntRect extension function. It only has roundToIntRect [1][2][3]. The function was previously named round, but it was renamed to roundToIntRect in earlier versions of Jetpack Compose [4]. You should use roundToIntRect to convert a Rect to an IntRect [1][3].
Citations:
- 1: https://developer.android.google.cn/reference/kotlin/androidx/compose/ui/geometry/Rect
- 2: https://composables.com/docs/androidx.compose.ui/ui-unit/functions/roundToIntRect
- 3: https://composables.com/jetpack-compose/androidx.compose.ui/ui-unit/functions/roundToIntRect/api
- 4: friendboy1/androidx@c3b44e1
Replace toIntRect() with roundToIntRect() coordinates.boundsInWindow() returns a Rect, and that type does not have toIntRect(), so this line will not compile. Use coordinates.boundsInWindow().roundToIntRect() instead.
🤖 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
`@balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonHost.kt`
around lines 155 - 159, The anchor bounds conversion in BalloonHost’s
onGloballyPositioned callback is using a nonexistent Rect conversion, so update
the coordinates.boundsInWindow() handling to use roundToIntRect() instead of
toIntRect(). Make this change in the code path that assigns entry.anchorBounds
so the bounds conversion compiles correctly.
| rectEnd: Float, | ||
| radius: Float, | ||
| ): Float { | ||
| val center = (fullExtent * ratio).coerceIn(halfArrow, fullExtent - halfArrow) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect callers to confirm no upstream guarantee that arrowWidthPx <= size extent.
rg -nP -C3 'arrowCenterAlong|arrowWidthPx|arrowSize' balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonShapeBuilder.ktRepository: skydoves/Balloon
Length of output: 3539
🏁 Script executed:
#!/bin/bash
sed -n '244,290p' balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonShapeBuilder.ktRepository: skydoves/Balloon
Length of output: 1482
Handle the degenerate case before coerceIn. coerceIn(halfArrow, fullExtent - halfArrow) throws when fullExtent < 2 * halfArrow, and the callers only guard against non-positive sizes. Clamp the bounds or fall back to the midpoint first so a small balloon with a wide arrow doesn’t crash here.
🤖 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
`@balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonShapeBuilder.kt`
at line 252, The center calculation in BalloonShapeBuilder should handle the
degenerate size case before calling coerceIn, because the current bounds can
become invalid when fullExtent is smaller than twice halfArrow. Update the logic
around the center computation to guard for this condition first and either clamp
the min/max bounds safely or fall back to the midpoint, so small balloons with
wide arrows don’t throw. Keep the fix localized to the center/arrow placement
logic in BalloonShapeBuilder.
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.
Support Kotlin Multiplatform.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes