Support Kotlin Multiplatform - #983
Open
skydoves wants to merge 4 commits into
Open
Conversation
WalkthroughAdds a Compose Multiplatform balloon library with shared state, positioning, overlays, animations, and builder APIs. It also adds shared demos, Android/Desktop/iOS/Wasm application wiring, API baselines, positioning tests, build configuration, and migration documentation. ChangesCompose Multiplatform Balloon
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Demo
participant BalloonState
participant BalloonHost
participant BalloonPopupLayer
participant BalloonContent
Demo->>BalloonState: show(...)
BalloonHost->>BalloonPopupLayer: register anchor and render popup
BalloonPopupLayer->>BalloonContent: render styled balloon
Demo->>BalloonState: dismiss()
BalloonPopupLayer->>BalloonHost: remove popup and overlay
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
+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.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonPopup.kt (1)
226-228: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRe-verify the
resolveArrowOrientationRTL fallback (previously flagged, fix not visible in this diff).A prior review flagged
resolveArrowOrientation'sBalloonAlign.START/ENDbranches for gating the logical-opposite-edge mapping onisRtl, whenArrowOrientationis itself a logical (START/END) value and the mapping should be RTL-independent (START -> END,END -> STARTunconditionally). This function is used as the pre-placement fallback here (line 228) and in the CENTER-overlay case ofcalculatePosition(line 473), so a lingering bug there still affects the first rendered frame and the CENTER-overlay path. The function body isn't included in this round's diff, so the fix status can't be confirmed from the provided context.#!/bin/bash # Inspect the current resolveArrowOrientation implementation. ast-grep run --pattern 'fun resolveArrowOrientation($$$) { $$$ }' --lang kotlin \ balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonPopup.ktAlso applies to: 470-474
🤖 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 226 - 228, Update resolveArrowOrientation so the BalloonAlign.START and END branches map to the logical opposite ArrowOrientation values unconditionally, without gating the mapping on isRtl. Verify this corrected function is used by both the resolvedOrientation fallback and the CENTER-overlay path in calculatePosition, preserving other orientation behavior.
♻️ Duplicate comments (1)
balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonStyle.kt (1)
97-133: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStill missing invariant validation on
BalloonStyle.
arrowPosition(documented as 0f..1f), and theLongduration fields (circularDurationMillis,highlightAnimationStartDelayMillis,autoDismissMillis) have no bounds checking in the constructor. A negativeautoDismissMillis, for instance, would be passed straight intodelay(style.autoDismissMillis)inBalloonPopupLayersince that call is guarded only by> 0L.🛡️ Proposed fix
val autoDismissMillis: Long = 0L, -) +) { + init { + require(arrowPosition in 0f..1f) { "arrowPosition must be in [0f, 1f]." } + require(circularDurationMillis >= 0L) { "circularDurationMillis must be >= 0." } + require(highlightAnimationStartDelayMillis >= 0L) { + "highlightAnimationStartDelayMillis must be >= 0." + } + require(autoDismissMillis >= 0L) { "autoDismissMillis must be >= 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/BalloonStyle.kt` around lines 97 - 133, Add constructor validation to BalloonStyle using an init block: require arrowPosition to remain within 0f..1f, and require circularDurationMillis, highlightAnimationStartDelayMillis, and autoDismissMillis to be non-negative. Keep all existing defaults and properties unchanged.
🤖 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/BalloonOverlay.kt`:
- Around line 155-190: Update the explicit-radius branch in the BalloonOverlay
drawing logic for BalloonOverlayShape.Circle so overlayPadding contributes to
the circle radius, while preserving the existing padded-rect fallback for
Dp.Unspecified. Use the computed pad consistently with shape.radius.toPx() and
leave other overlay shapes unchanged.
In
`@samples-shared/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/sample/BalloonDemoScreen.kt`:
- Around line 1132-1172: Update the Icon call in BottomNavItem to set
contentDescription to null, matching the other icon-and-text components in the
file and leaving the adjacent visible label as the sole accessibility
announcement.
---
Outside diff comments:
In
`@balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonPopup.kt`:
- Around line 226-228: Update resolveArrowOrientation so the BalloonAlign.START
and END branches map to the logical opposite ArrowOrientation values
unconditionally, without gating the mapping on isRtl. Verify this corrected
function is used by both the resolvedOrientation fallback and the CENTER-overlay
path in calculatePosition, preserving other orientation behavior.
---
Duplicate comments:
In
`@balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonStyle.kt`:
- Around line 97-133: Add constructor validation to BalloonStyle using an init
block: require arrowPosition to remain within 0f..1f, and require
circularDurationMillis, highlightAnimationStartDelayMillis, and
autoDismissMillis to be non-negative. Keep all existing defaults and properties
unchanged.
🪄 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 Plus
Run ID: c38d3d26-5ef4-4bc2-8f2d-062c7eca1289
⛔ Files ignored due to path filters (1)
samples-shared/src/commonMain/composeResources/drawable/sample0.pngis excluded by!**/*.png
📒 Files selected for processing (21)
androidApp/src/androidMain/AndroidManifest.xmlandroidApp/src/androidMain/kotlin/com/skydoves/balloon/kmpdemo/KmpBalloonActivity.ktandroidApp/src/androidMain/res/values/styles.xmlballoon-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/BalloonEasings.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonHighlightAnimation.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonHost.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonOverlay.ktballoon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonPopup.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.ktsamples-shared/build.gradle.ktssamples-shared/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/sample/BalloonDemoScreen.ktsamples-shared/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/sample/DemoIcons.kt
🚧 Files skipped from review as they are similar to previous changes (8)
- androidApp/src/androidMain/AndroidManifest.xml
- balloon-compose-multiplatform/src/androidMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonPopupProperties.android.kt
- androidApp/src/androidMain/kotlin/com/skydoves/balloon/kmpdemo/KmpBalloonActivity.kt
- samples-shared/build.gradle.kts
- balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonShapeBuilder.kt
- balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/Balloon.kt
- balloon-compose-multiplatform/src/commonTest/kotlin/com/skydoves/balloon/compose/multiplatform/PositionProviderTest.kt
- balloon-compose-multiplatform/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/BalloonState.kt
Comment on lines
+155
to
+190
| val pad = style.overlayPadding.toPx() | ||
| val rect = Rect( | ||
| left = (anchorBounds.left - originInWindow.x) - pad, | ||
| top = (anchorBounds.top - originInWindow.y) - pad, | ||
| right = (anchorBounds.right - originInWindow.x) + pad, | ||
| bottom = (anchorBounds.bottom - originInWindow.y) + pad, | ||
| ) | ||
| if (rect.width <= 0f || rect.height <= 0f) return | ||
|
|
||
| when (val shape = style.overlayShape) { | ||
| BalloonOverlayShape.Empty -> Unit | ||
|
|
||
| BalloonOverlayShape.Rect -> drawRect( | ||
| color = Color.Transparent, | ||
| topLeft = rect.topLeft, | ||
| size = rect.size, | ||
| blendMode = BlendMode.Clear, | ||
| ) | ||
|
|
||
| BalloonOverlayShape.Oval -> drawOval( | ||
| color = Color.Transparent, | ||
| topLeft = rect.topLeft, | ||
| size = rect.size, | ||
| blendMode = BlendMode.Clear, | ||
| ) | ||
|
|
||
| is BalloonOverlayShape.Circle -> drawCircle( | ||
| color = Color.Transparent, | ||
| radius = if (shape.radius == Dp.Unspecified) { | ||
| max(rect.width, rect.height) / 2f | ||
| } else { | ||
| shape.radius.toPx() | ||
| }, | ||
| center = rect.center, | ||
| blendMode = BlendMode.Clear, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
overlayPadding is ignored for an explicit Circle(radius = ...).
The padded rect (lines 156-161) is only used for the Dp.Unspecified fallback radius (max(rect.width, rect.height) / 2f). When a caller supplies an explicit radius, shape.radius.toPx() is used directly and pad is dropped, so overlayPadding silently has no effect for that case, unlike every other shape.
🐛 Proposed fix
is BalloonOverlayShape.Circle -> drawCircle(
color = Color.Transparent,
radius = if (shape.radius == Dp.Unspecified) {
max(rect.width, rect.height) / 2f
} else {
- shape.radius.toPx()
+ shape.radius.toPx() + pad
},
center = rect.center,
blendMode = BlendMode.Clear,
)📝 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 pad = style.overlayPadding.toPx() | |
| val rect = Rect( | |
| left = (anchorBounds.left - originInWindow.x) - pad, | |
| top = (anchorBounds.top - originInWindow.y) - pad, | |
| right = (anchorBounds.right - originInWindow.x) + pad, | |
| bottom = (anchorBounds.bottom - originInWindow.y) + pad, | |
| ) | |
| if (rect.width <= 0f || rect.height <= 0f) return | |
| when (val shape = style.overlayShape) { | |
| BalloonOverlayShape.Empty -> Unit | |
| BalloonOverlayShape.Rect -> drawRect( | |
| color = Color.Transparent, | |
| topLeft = rect.topLeft, | |
| size = rect.size, | |
| blendMode = BlendMode.Clear, | |
| ) | |
| BalloonOverlayShape.Oval -> drawOval( | |
| color = Color.Transparent, | |
| topLeft = rect.topLeft, | |
| size = rect.size, | |
| blendMode = BlendMode.Clear, | |
| ) | |
| is BalloonOverlayShape.Circle -> drawCircle( | |
| color = Color.Transparent, | |
| radius = if (shape.radius == Dp.Unspecified) { | |
| max(rect.width, rect.height) / 2f | |
| } else { | |
| shape.radius.toPx() | |
| }, | |
| center = rect.center, | |
| blendMode = BlendMode.Clear, | |
| ) | |
| val pad = style.overlayPadding.toPx() | |
| val rect = Rect( | |
| left = (anchorBounds.left - originInWindow.x) - pad, | |
| top = (anchorBounds.top - originInWindow.y) - pad, | |
| right = (anchorBounds.right - originInWindow.x) + pad, | |
| bottom = (anchorBounds.bottom - originInWindow.y) + pad, | |
| ) | |
| if (rect.width <= 0f || rect.height <= 0f) return | |
| when (val shape = style.overlayShape) { | |
| BalloonOverlayShape.Empty -> Unit | |
| BalloonOverlayShape.Rect -> drawRect( | |
| color = Color.Transparent, | |
| topLeft = rect.topLeft, | |
| size = rect.size, | |
| blendMode = BlendMode.Clear, | |
| ) | |
| BalloonOverlayShape.Oval -> drawOval( | |
| color = Color.Transparent, | |
| topLeft = rect.topLeft, | |
| size = rect.size, | |
| blendMode = BlendMode.Clear, | |
| ) | |
| is BalloonOverlayShape.Circle -> drawCircle( | |
| color = Color.Transparent, | |
| radius = if (shape.radius == Dp.Unspecified) { | |
| max(rect.width, rect.height) / 2f | |
| } else { | |
| shape.radius.toPx() + pad | |
| }, | |
| center = rect.center, | |
| blendMode = BlendMode.Clear, | |
| ) |
🤖 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/BalloonOverlay.kt`
around lines 155 - 190, Update the explicit-radius branch in the BalloonOverlay
drawing logic for BalloonOverlayShape.Circle so overlayPadding contributes to
the circle radius, while preserving the existing padded-rect fallback for
Dp.Unspecified. Use the computed pad consistently with shape.radius.toPx() and
leave other overlay shapes unchanged.
Comment on lines
+1132
to
+1172
| @Composable | ||
| private fun BottomNavItem( | ||
| icon: ImageVector, | ||
| label: String, | ||
| style: BalloonStyle, | ||
| tagText: String, | ||
| onMessage: (String) -> Unit, | ||
| ) { | ||
| val balloonState = rememberBalloonState(style) | ||
|
|
||
| Column( | ||
| horizontalAlignment = Alignment.CenterHorizontally, | ||
| modifier = Modifier | ||
| .balloon(balloonState) { | ||
| Text( | ||
| text = tagText, | ||
| color = Background, | ||
| fontSize = 12.sp, | ||
| fontWeight = FontWeight.Medium, | ||
| ) | ||
| } | ||
| .clickable { | ||
| balloonState.showAlignTop() | ||
| onMessage("$label clicked") | ||
| } | ||
| .padding(horizontal = 16.dp, vertical = 4.dp), | ||
| ) { | ||
| Icon( | ||
| imageVector = icon, | ||
| contentDescription = label, | ||
| tint = White93, | ||
| modifier = Modifier.size(24.dp), | ||
| ) | ||
| Spacer(modifier = Modifier.height(2.dp)) | ||
| Text( | ||
| text = label, | ||
| color = White93, | ||
| fontSize = 10.sp, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Redundant contentDescription on BottomNavItem's icon.
The icon sets contentDescription = label (Line 1161) while the adjacent visible Text(label) (lines 1166-1170) already exposes the same string, causing duplicate screen-reader announcements. Every other icon+text pairing in this file (MenuItem Line 267, LazyColumnHeader Line 951, ListItemWithBalloon Line 1074) nulls out contentDescription for exactly this reason.
♿ Proposed fix
Icon(
imageVector = icon,
- contentDescription = label,
+ contentDescription = null,
tint = White93,
modifier = Modifier.size(24.dp),
)📝 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
| @Composable | |
| private fun BottomNavItem( | |
| icon: ImageVector, | |
| label: String, | |
| style: BalloonStyle, | |
| tagText: String, | |
| onMessage: (String) -> Unit, | |
| ) { | |
| val balloonState = rememberBalloonState(style) | |
| Column( | |
| horizontalAlignment = Alignment.CenterHorizontally, | |
| modifier = Modifier | |
| .balloon(balloonState) { | |
| Text( | |
| text = tagText, | |
| color = Background, | |
| fontSize = 12.sp, | |
| fontWeight = FontWeight.Medium, | |
| ) | |
| } | |
| .clickable { | |
| balloonState.showAlignTop() | |
| onMessage("$label clicked") | |
| } | |
| .padding(horizontal = 16.dp, vertical = 4.dp), | |
| ) { | |
| Icon( | |
| imageVector = icon, | |
| contentDescription = label, | |
| tint = White93, | |
| modifier = Modifier.size(24.dp), | |
| ) | |
| Spacer(modifier = Modifier.height(2.dp)) | |
| Text( | |
| text = label, | |
| color = White93, | |
| fontSize = 10.sp, | |
| ) | |
| } | |
| } | |
| `@Composable` | |
| private fun BottomNavItem( | |
| icon: ImageVector, | |
| label: String, | |
| style: BalloonStyle, | |
| tagText: String, | |
| onMessage: (String) -> Unit, | |
| ) { | |
| val balloonState = rememberBalloonState(style) | |
| Column( | |
| horizontalAlignment = Alignment.CenterHorizontally, | |
| modifier = Modifier | |
| .balloon(balloonState) { | |
| Text( | |
| text = tagText, | |
| color = Background, | |
| fontSize = 12.sp, | |
| fontWeight = FontWeight.Medium, | |
| ) | |
| } | |
| .clickable { | |
| balloonState.showAlignTop() | |
| onMessage("$label clicked") | |
| } | |
| .padding(horizontal = 16.dp, vertical = 4.dp), | |
| ) { | |
| Icon( | |
| imageVector = icon, | |
| contentDescription = null, | |
| tint = White93, | |
| modifier = Modifier.size(24.dp), | |
| ) | |
| Spacer(modifier = Modifier.height(2.dp)) | |
| Text( | |
| text = label, | |
| color = White93, | |
| fontSize = 10.sp, | |
| ) | |
| } | |
| } |
🤖 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
`@samples-shared/src/commonMain/kotlin/com/skydoves/balloon/compose/multiplatform/sample/BalloonDemoScreen.kt`
around lines 1132 - 1172, Update the Icon call in BottomNavItem to set
contentDescription to null, matching the other icon-and-text components in the
file and leaving the adjacent visible label as the sole accessibility
announcement.
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
Tests