Conversation
- Introduce CloudyState.Success.Applied for GPU blur (no bitmap) - Introduce CloudyState.Success.Captured for CPU blur (with bitmap) - Update KDoc and expect functions
- Use RenderEffect.createBlurEffect() on Android 12+ (API 31+) - Retain Native C++ fallback for API 30- - Return CloudyState.Success.Applied for RenderEffect path - Add Robolectric dependency for version branching tests - Update unit tests for new state hierarchy
- Use Skia BlurEffect (backed by Metal) for high-performance blur - Remove placeholder bitmap generation and CPU readback - Return CloudyState.Success.Applied (no bitmap) - Update iOS unit tests - Update iOS app project configuration and bundle identifiers
- Verify RenderEffect condition on API 31+ - Verify Native path condition on API 30- - Verify CloudyState subtypes behavior - Update README with platform support details and migration guide
WalkthroughSplit CloudyState.Success into two subtypes (Applied and Captured), introduced Android strategy-based rendering (RenderEffect for API ≥31, legacy bitmap fallback for <31), simplified iOS to GPU Skia blur, rewrote demo and theming, added tests and test utilities, and renamed frameworks to CloudyApp. Changes
Sequence Diagram(s)sequenceDiagram
actor App
participant Modifier as Modifier.cloudy
participant PlatformCheck as SDK/Platform Check
participant RenderStrategy as RenderEffectStrategy
participant LegacyStrategy as LegacyBlurStrategy
participant onState as onStateChanged
App->>Modifier: apply blur(radius)
Modifier->>PlatformCheck: determine platform / SDK
alt SDK >= 31 or iOS GPU
PlatformCheck->>RenderStrategy: select GPU strategy
RenderStrategy->>RenderStrategy: compute sigma & apply RenderEffect/BlurEffect
RenderStrategy->>onState: emit Success.Applied
else Android <31 (legacy)
PlatformCheck->>LegacyStrategy: select legacy strategy
LegacyStrategy->>LegacyStrategy: capture bitmap & start async blur
LegacyStrategy->>onState: emit Loading
LegacyStrategy->>LegacyStrategy: finish -> emit Success.Captured(bitmap) or Error
end
onState-->>App: deliver state update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~85 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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: 1
🧹 Nitpick comments (5)
cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt (1)
52-77: Consider guardingonStateChangedagainst repeated calls from recomposition
onStateChanged(CloudyState.Success.Applied)is invoked directly in the composable body, so it will run on every recomposition with the sameradius/enabledinputs; if callers update Compose state in this callback, this can create unnecessary cycles. Consider moving this into aSideEffector tracking the last emitted state so the callback only fires when the state actually changes.cloudy/src/iosTest/kotlin/com/skydoves/cloudy/CloudyStateTest.kt (1)
66-90: Consider simplifying the pattern matching test.The test verifies pattern matching works but uses mutable flags with empty branches. A more idiomatic approach would use exhaustive
whenexpressions directly.@Test fun successTypeHierarchyAllowsPatternMatchingBothSubtypes() { val appliedState: CloudyState = CloudyState.Success.Applied val capturedState: CloudyState = CloudyState.Success.Captured( createTestPlatformBitmap(100, 100), ) - var appliedMatched = false - var capturedMatched = false - - when (appliedState) { - is CloudyState.Success.Applied -> appliedMatched = true - is CloudyState.Success.Captured -> {} - else -> {} - } - - when (capturedState) { - is CloudyState.Success.Captured -> capturedMatched = true - is CloudyState.Success.Applied -> {} - else -> {} - } - - assertTrue(appliedMatched) - assertTrue(capturedMatched) + val appliedMatched = when (appliedState) { + is CloudyState.Success.Applied -> true + else -> false + } + + val capturedMatched = when (capturedState) { + is CloudyState.Success.Captured -> true + else -> false + } + + assertTrue(appliedMatched) + assertTrue(capturedMatched) }cloudy/src/commonMain/kotlin/com/skydoves/cloudy/CloudyState.kt (1)
59-60: Consider using@Immutableinstead of@Stablefor the sealed interface.Both subtypes (
AppliedandCaptured) are marked@Immutable. Since the sealed interface itself has no mutable state and all implementations are immutable,@Immutablewould be more precise and consistent.- @Stable + @Immutable public sealed interface Success : CloudyState {cloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/CloudyModifierTest.kt (1)
204-211: Consider extracting duplicatecreateMockPlatformBitmapto shared test utilities.This helper is duplicated in
CloudyStateTest.kt(lines 147-155 per the relevant snippets). Consider extracting it to a shared test utilities file to reduce duplication and simplify maintenance.cloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.kt (1)
121-127: Redundant API level check inside graphicsLayer.The
Build.VERSION.SDK_INT >= Build.VERSION_CODES.Scheck on line 122 is redundant sincecloudyWithRenderEffectis only called when this condition is already true (line 88). However, this may be intentional for lint suppression.If kept for lint compliance, consider adding a comment explaining why:
return this.graphicsLayer { // Note: API check retained for lint compliance; this function is only called on API 31+ if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
README.md(2 hunks)app/build.gradle.kts(2 hunks)app/src/commonMain/kotlin/demo/theme/Color.kt(1 hunks)app/src/commonMain/kotlin/demo/theme/PosterTheme.kt(1 hunks)cloudy/api/cloudy.api(1 hunks)cloudy/build.gradle.kts(1 hunks)cloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.kt(7 hunks)cloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/CloudyModifierTest.kt(1 hunks)cloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/CloudyStateTest.kt(3 hunks)cloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.kt(1 hunks)cloudy/src/commonMain/kotlin/com/skydoves/cloudy/CloudyState.kt(1 hunks)cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt(2 hunks)cloudy/src/iosTest/kotlin/com/skydoves/cloudy/CloudyStateTest.kt(2 hunks)gradle/libs.versions.toml(2 hunks)iosApp/iosApp.xcodeproj/project.pbxproj(5 hunks)iosApp/iosApp/ContentView.swift(1 hunks)iosApp/iosApp/Info.plist(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: l2hyunwoo
Repo: skydoves/Cloudy PR: 63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.820Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
📚 Learning: 2025-07-06T06:13:55.820Z
Learnt from: l2hyunwoo
Repo: skydoves/Cloudy PR: 63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.820Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
Applied to files:
README.mdcloudy/src/iosTest/kotlin/com/skydoves/cloudy/CloudyStateTest.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/CloudyState.ktcloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.ktcloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/CloudyModifierTest.ktcloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/CloudyStateTest.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.ktcloudy/api/cloudy.api
🧬 Code graph analysis (3)
cloudy/src/iosTest/kotlin/com/skydoves/cloudy/CloudyStateTest.kt (1)
cloudy/src/iosTest/kotlin/com/skydoves/cloudy/TestUtils.kt (1)
createTestPlatformBitmap(29-30)
cloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/CloudyModifierTest.kt (1)
cloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/CloudyStateTest.kt (1)
createMockPlatformBitmap(148-156)
cloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/CloudyStateTest.kt (1)
cloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/CloudyModifierTest.kt (1)
createMockPlatformBitmap(204-211)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: API check
- GitHub Check: Spotless check
🔇 Additional comments (26)
iosApp/iosApp/Info.plist (1)
49-50: Enabling higher frame rates on iPhone looks appropriateAdding
CADisableMinimumFrameDurationOnPhone = trueis valid here and aligns well with the GPU blur work (smoother animations on ProMotion devices). Just be aware this opts the whole app into potentially higher frame rates, which can slightly increase power usage, but there’s no compatibility downside—older iOS versions will simply ignore the key.app/src/commonMain/kotlin/demo/theme/PosterTheme.kt (2)
24-34: Dark theme palette wiring looks consistentAll dark palette slots are wired to the new Disney-themed tokens in a consistent way (dark background/surface, light on-colors, bright secondary). No functional or semantic issues spotted.
36-45: Light theme palette wiring is coherent with the new tokensLight palette correctly mirrors the dark palette using the light background/surface tokens and appropriate on-colors; using
backgroundDarkasonSecondaryshould give good contrast on the gold secondary. No changes needed.app/src/commonMain/kotlin/demo/theme/Color.kt (1)
20-39: New Disney-inspired color tokens are well-structuredThe new color constants are valid, scoped appropriately (
internal), and line up with howPosterThemeconsumes them (background/surface/on-background + primary/secondary). This is a clean, forward-compatible tokenization of the palette.iosApp/iosApp/ContentView.swift (1)
3-3: Import rename toCloudyAppis consistent with framework baseName changesThe updated import aligns with the new Kotlin/Native framework name and the Xcode link flags; no issues here.
cloudy/build.gradle.kts (1)
137-146: Robolectric dependency wiring forandroidUnitTestlooks correctAdding
implementation(libs.robolectric)toandroidUnitTest.dependenciesmatches the existing pattern for other test libs and is appropriate for JVM-based Android unit tests.Please confirm locally that
./gradlew :cloudy:androidUnitTestruns successfully with this Robolectric version alongside your AGP/Kotlin toolchain.app/build.gradle.kts (1)
30-48: FrameworkbaseNamerename toCloudyAppis coherent across targetsUsing
baseName = "CloudyApp"for all Kotlin/Native frameworks matches the updated Xcode project and Swift import; this keeps the iOS integration consistent.gradle/libs.versions.toml (1)
28-28: Robolectric version and library alias are wired correctlyThe new
robolectricversion entry and corresponding library alias are consistent and correctly referenced from the Android unit test source set.Please double-check that Robolectric
4.14.1is compatible with your AGP and AndroidX test stack in this repo’s toolchain.Also applies to: 77-77
iosApp/iosApp.xcodeproj/project.pbxproj (1)
139-156: Xcode embedding & linking now correctly target theCloudyAppframeworkThe embed script, framework search paths, and
OTHER_LDFLAGSentries consistently point to the app module’sCloudyAppframework underapp/build/xcode-frameworks, which matches the Gradle and Swift-side renames.Also applies to: 291-309, 319-338
cloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.kt (1)
21-76: Updated KDoc accurately reflects the new GPU/CPU blur behavior and state modelThe added documentation clearly explains platform-specific implementations, the
Success.AppliedvsSuccess.Capturedsplit, and radius→sigma handling, while keeping the public API stable; this should make the new state model much easier to consume.cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt (1)
21-45: iOS GPU blur path and state signaling are consistent with the new designUsing Skia
BlurEffectthroughgraphicsLayer(withsigma = radius / 2.0f) and always reportingCloudyState.Success.Applied(no bitmap) cleanly aligns iOS with the GPU-only success path described in the common KDoc and removes dependence on the previous bitmap extraction path.Also applies to: 52-77
cloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/CloudyStateTest.kt (1)
22-100: New tests give solid coverage of theCloudyState.SuccesshierarchyThe added assertions around
Success.Appliedsingleton behavior,Success.Capturedbitmap containment, subtype checks, pattern matching, and equality semantics collectively provide good safety nets for the new state design.Also applies to: 111-125
cloudy/src/iosTest/kotlin/com/skydoves/cloudy/CloudyStateTest.kt (3)
25-44: LGTM! Good singleton verification tests.The tests correctly verify that
Nothing,Loading, andSuccess.Appliedare singletons using referential equality (===).
46-64: LGTM! State subtype tests are well-structured.Tests correctly verify that
Capturedholds a bitmap and both subtypes are instances ofSuccess.
99-114: LGTM! Equality tests cover the expected scenarios.Tests correctly verify that
Capturedstates with the same bitmap are equal and those with different bitmaps are not.README.md (3)
13-25: LGTM! Clear platform support documentation.The table effectively communicates the blur implementation strategy per platform, aligning with the PR objectives.
114-139: LGTM! Code example demonstrates proper state handling.The example clearly shows how to handle both
Success.Applied(GPU) andSuccess.Captured(CPU with bitmap) states with appropriate comments for each platform behavior.
141-150: LGTM! State types table provides clear reference.The table succinctly documents bitmap availability for each state, which is crucial for users migrating from the previous API.
cloudy/src/commonMain/kotlin/com/skydoves/cloudy/CloudyState.kt (1)
59-84: LGTM! Well-designed state hierarchy.The sealed interface with
AppliedandCapturedsubtypes clearly models the GPU vs CPU blur paths. The documentation is thorough, explaining platform-specific behavior and when each subtype is used.cloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/CloudyModifierTest.kt (2)
44-112: LGTM! Comprehensive API version branching tests.The tests effectively verify the RenderEffect condition logic across different API levels (S, S_V2, TIRAMISU, R, Q), ensuring the correct path selection.
182-202: LGTM! Exhaustive when expression test ensures type safety.This test validates that all
CloudyStatevariants can be matched exhaustively, which is valuable for detecting breaking changes if new states are added.cloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.kt (3)
87-97: LGTM! Clean API level branching.The conditional routing to RenderEffect (API 31+) vs native CPU blur (API 30-) is clear and follows Android best practices for feature detection.
174-178: Inconsistent state forradius <= 0in CPU path.When
radius <= 0, the CPU path emitsSuccess.Appliedbut no blur is actually applied—the content is just drawn as-is. While this matches the GPU path behavior, emittingApplied(which semantically means "GPU blur applied") for a no-op case in the CPU path could be confusing.Consider whether
Nothingwould be more semantically accurate for "no blur requested", or document this behavior explicitly.
182-216: LGTM! Proper coroutine and resource management.The CPU blur path correctly:
- Launches on
Dispatchers.Main.immediatefor UI updates- Handles bitmap caching and disposal
- Releases graphics layer in
finallyblock- Emits
Success.Capturedwith the blurred bitmapcloudy/api/cloudy.api (2)
32-41: Well-structured sealed hierarchy for GPU/CPU blur distinction.The
Successsealed interface withApplied(singleton for GPU path) andCaptured(data class for CPU path with bitmap) is a clean design that clearly communicates which blur strategy was used and whether a bitmap is available. This properly addresses the iOS placeholder behavior mentioned in prior learnings.
43-53: Breaking API change: verify migration documentation is in place.The rename from
CloudyState.Success(bitmap)toCloudyState.Success.Captured(bitmap)is a binary-incompatible change. Consumers pattern-matching on the oldSuccesstype will need code updates.Ensure the README or CHANGELOG includes migration guidance, particularly for the pattern matching change:
// Before when (state) { is CloudyState.Success -> state.bitmap } // After when (state) { is CloudyState.Success.Captured -> state.bitmap is CloudyState.Success.Applied -> /* GPU path, no bitmap */ }
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @l2hyunwoo. * #78 (comment) The following files were modified: * `app/src/commonMain/kotlin/demo/theme/PosterTheme.kt` * `cloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.kt` * `cloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.kt` * `cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt` * `iosApp/iosApp/ContentView.swift`
Docstrings generation was requested by @l2hyunwoo. * #78 (comment) The following files were modified: * `app/src/commonMain/kotlin/demo/theme/PosterTheme.kt` * `cloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.kt` * `cloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.kt` * `cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt` * `iosApp/iosApp/ContentView.swift` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.kt (1)
123-146: Previous review feedback not addressed.The side effect issue with
onStateChanged.invoke()being called directly during composition (lines 129 and 137) was already flagged in a previous review. This should be wrapped inSideEffectorLaunchedEffectto prevent execution on every recomposition.
🧹 Nitpick comments (2)
app/src/commonMain/kotlin/demo/theme/PosterTheme.kt (1)
59-79: PosterTheme implementation looks idiomatic; consider visibility intentThe
PosterThemecomposable correctly switches palettes and typography based ondarkThemeand delegates toMaterialThemein an idiomatic way. If you ever plan to reuse this theme entry point from outside the demo module, you might want to relaxinternal; otherwise this scope is fine.cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt (1)
25-59: Consolidate duplicate KDoc blocks.Two KDoc blocks document the same function with overlapping content. The first block (lines 25-45) and second block (lines 46-59) both describe the iOS blur implementation, sigma conversion, and state behavior. Merge them into a single, comprehensive documentation block to improve maintainability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/commonMain/kotlin/demo/theme/PosterTheme.kt(2 hunks)cloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.kt(7 hunks)cloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.kt(1 hunks)cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt(2 hunks)iosApp/iosApp/ContentView.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.kt
- iosApp/iosApp/ContentView.swift
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: l2hyunwoo
Repo: skydoves/Cloudy PR: 63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.820Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
📚 Learning: 2025-07-06T06:13:55.820Z
Learnt from: l2hyunwoo
Repo: skydoves/Cloudy PR: 63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.820Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
Applied to files:
cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Spotless check
- GitHub Check: build
- GitHub Check: API check
🔇 Additional comments (4)
app/src/commonMain/kotlin/demo/theme/PosterTheme.kt (2)
24-46: Updated color palettes are wired cleanly to the new token setDark/light palettes use the new background/surface and disney* tokens consistently, and map on* colors in a coherent way for MaterialTheme usage. No functional issues spotted here.
48-57: KDoc forPosterThemeis clear and sufficientThe documentation accurately describes behavior, parameters, and defaulting to
isSystemInDarkTheme(), which should make usage straightforward.cloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.kt (2)
210-214: Consider side effect implications in draw phase.While
onStateChangedis called in the draw phase (not during composition), repeated draw calls will trigger the callback multiple times forradius <= 0. Consider whether this is intentional or if you need to gate the callback to fire only once per radius change.
245-246: LGTM: Correct state handling for CPU path.The implementation correctly uses
CloudyState.Success.Captured(blurredBitmap)for the CPU-based blur path, aligning with the PR's redesign of CloudyState into distinct GPU (Applied) and CPU (Captured) subtypes.
Updates README to reflect the new GPU/CPU blur implementations and CloudyState types.
- Refactor `cloudy` modifier to delegate to `CloudyRenderEffectStrategy` or `CloudyLegacyBlurStrategy` based on API level. - Add `debugTag` parameter to `cloudy` modifier for easier debugging. - Implement `CloudyRenderEffectStrategy` using RenderEffect (API 31+). - Implement `CloudyLegacyBlurStrategy` with optimized bitmap capture and iterative blur (API < 31). - Add support for `GraphicsLayer` capture in `androidMain` source set. - Update demo app with new API and UI improvements.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt (1)
44-56: UnuseddebugTagparameter.The
debugTagparameter is accepted but never used in this strategy. Consider either adding logging support or removing the parameter if it's not needed for the legacy path.@Composable override fun apply( modifier: Modifier, radius: Int, onStateChanged: (CloudyState) -> Unit, debugTag: String, ): Modifier = modifier.then( CloudyModifierNodeElement( radius = radius, onStateChanged = onStateChanged, + debugTag = debugTag, ), )If logging is added later, pass it through to the node element.
cloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.kt (1)
66-72: Missing@paramdocumentation fordebugTag.The new
debugTagparameter at line 92 is not documented in the KDoc block. Add documentation to describe its purpose.Add to the
@paramsection:* @param debugTag Optional identifier for debugging and logging purposes.cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyRenderEffectStrategy.kt (1)
37-37: UnuseddebugTagparameter.Same as the legacy strategy,
debugTagis accepted but not used. Consider adding logging support or documenting it as reserved for future use.cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt (1)
66-66: UnuseddebugTagparameter.The
debugTagparameter is added for API consistency with the common signature but is not used in the iOS implementation. Consider suppressing the warning or adding a comment explaining it's reserved for future use / platform parity.public actual fun Modifier.cloudy( @IntRange(from = 0) radius: Int, enabled: Boolean, onStateChanged: (CloudyState) -> Unit, + @Suppress("UNUSED_PARAMETER") // Reserved for debugging; unused on iOS debugTag: String, ): Modifier {androidMain/androidx/compose/ui/graphics/AndroidGraphicsContext.android.kt (2)
123-135: Broad exception handling may hide unrelated failures.Catching
Throwable(line 125) is overly broad and could swallowOutOfMemoryError,StackOverflowError, or other critical errors that shouldn't trigger the fallback logic.Consider catching a more specific exception type:
try { GraphicsLayerV23(ownerView, ownerId) - } catch (_: Throwable) { + } catch (_: Exception) { // If we ever failed to create an instance of the RenderNode stub // based GraphicsLayer, always fallback to creation of View based layersNote: If this is directly copied from AOSP, you may want to keep parity with upstream; in that case, add a comment noting the intentional broad catch.
139-140: Redundant local variable.The local
layervariable is assigned and immediately returned.- val layer = GraphicsLayer(layerImpl) - return layer + return GraphicsLayer(layerImpl)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
androidMain/androidx/compose/ui/graphics/AndroidGraphicsContext.android.kt(1 hunks)androidMain/androidx/compose/ui/graphics/layer/GraphicsLayerV29.android.kt(1 hunks)app/src/commonMain/kotlin/demo/Main.kt(2 hunks)cloudy/api/cloudy.api(1 hunks)cloudy/build.gradle.kts(2 hunks)cloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.kt(1 hunks)cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyBlurStrategy.kt(1 hunks)cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt(1 hunks)cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyRenderEffectStrategy.kt(1 hunks)cloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.kt(1 hunks)cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cloudy/build.gradle.kts
- app/src/commonMain/kotlin/demo/Main.kt
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: l2hyunwoo
Repo: skydoves/Cloudy PR: 63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.820Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
📚 Learning: 2025-07-06T06:13:55.820Z
Learnt from: l2hyunwoo
Repo: skydoves/Cloudy PR: 63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.820Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
Applied to files:
cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyBlurStrategy.ktcloudy/api/cloudy.apicloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyRenderEffectStrategy.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Spotless check
- GitHub Check: build
- GitHub Check: API check
🔇 Additional comments (9)
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyBlurStrategy.kt (1)
27-44: Well-designed strategy interface.The interface cleanly abstracts the blur implementation details, enabling runtime strategy selection based on API level. The
@Composableannotation onapplyis appropriate since implementations will use Compose-specific APIs.androidMain/androidx/compose/ui/graphics/layer/GraphicsLayerV29.android.kt (2)
1-50: Verify AOSP code attribution and licensing.This file has a different copyright header ("The Android Open Source Project") compared to other Cloudy files ("skydoves"). If this is adapted from Jetpack Compose sources, ensure proper attribution and verify that incorporating it complies with the Apache 2.0 license terms (which it should, but the origin should be documented).
Consider adding a comment indicating the source, e.g.:
/** * GraphicsLayer implementation for Android Q+ that uses the public RenderNode API. * * Adapted from AndroidX Compose UI graphics layer implementation. */
235-257: LGTM - Recording implementation.The
record()method correctly wraps the RenderNode recording with proper try-finally to ensureendRecording()is always called. The CanvasDrawScope configuration is properly set up.cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt (2)
74-77: State signaling viaLaunchedEffectis correct.Using
LaunchedEffect(radius)properly scopes theonStateChangedcallback to avoid side effects during composition. This correctly addresses the previous review feedback about composition-time side effects.
83-94: GPU blur implementation looks good.The sigma conversion (
radius / 2.0f) and BlurEffect application viagraphicsLayerare correctly implemented. The use ofTileMode.Clampfor edge treatment is appropriate.cloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.kt (2)
43-45: Inspection mode handling is appropriate.Using the simple
blurmodifier for preview/inspection mode is a pragmatic fallback that ensures tooling support without full strategy initialization.
47-58: Clean strategy pattern implementation.The API-level branching logic is well-structured. Using
Build.VERSION_CODES.S(API 31) as the threshold correctly delegates toCloudyRenderEffectStrategyfor GPU-accelerated blur on newer devices andCloudyLegacyBlurStrategyfor bitmap-based blur on API 30 and below. Both strategies properly implement theCloudyBlurStrategyinterface with the expected@Composable override fun applysignature.androidMain/androidx/compose/ui/graphics/AndroidGraphicsContext.android.kt (1)
17-17: Vendoring AOSP code intoandroidx.compose.ui.graphicspackage.This file is placed under the
androidx.compose.ui.graphicspackage, which is typically owned by Jetpack Compose. Vendoring framework code can lead to maintenance burden and potential conflicts with the actual library.Please clarify:
- Is this necessary because the required APIs aren't exposed publicly?
- Consider adding a comment explaining why this is vendored and from which Compose version.
- If this is for internal testing or compatibility, consider an internal package namespace to avoid confusion.
cloudy/api/cloudy.api (1)
32-53: API surface correctly reflects theCloudyState.Successredesign.The breaking change from
CloudyState.Successas a data class to a sealed interface withAppliedandCapturedsubtypes is properly reflected in the API file. This aligns with the PR objectives for distinguishing GPU blur (no bitmap) from CPU blur (with bitmap).Consumers will need to update pattern matching from:
// Before is CloudyState.Success -> state.bitmap // After is CloudyState.Success.Captured -> state.bitmap is CloudyState.Success.Applied -> // no bitmap available
| internal companion object { | ||
| var isRenderNodeCompatible: Boolean = true | ||
| } |
There was a problem hiding this comment.
Mutable companion object state without synchronization.
isRenderNodeCompatible is a mutable var in a companion object that can be modified from createGraphicsLayer(). While createGraphicsLayer uses synchronized(lock), the companion object is shared across all AndroidGraphicsContext instances, and lock is an instance field—not a class-level lock.
If multiple AndroidGraphicsContext instances exist, concurrent access to isRenderNodeCompatible is unsynchronized:
internal companion object {
- var isRenderNodeCompatible: Boolean = true
+ @Volatile
+ var isRenderNodeCompatible: Boolean = true
}Alternatively, use AtomicBoolean or a class-level lock object for the companion.
🤖 Prompt for AI Agents
In androidMain/androidx/compose/ui/graphics/AndroidGraphicsContext.android.kt
around lines 170 to 172, the companion object exposes a mutable var
isRenderNodeCompatible that can be written from instance-level synchronized code
causing race conditions; replace the mutable companion var with a thread-safe
alternative (preferred: java.util.concurrent.atomic.AtomicBoolean) or use a
class-level lock in the companion. Concretely, change isRenderNodeCompatible to
an AtomicBoolean and update all reads/writes to use get()/set()/compareAndSet(),
or declare a private @JvmStatic final lock object inside the companion and
synchronize on that lock for all accesses to isRenderNodeCompatible so all
instances marshal access consistently.
|
|
||
| public final class com/skydoves/cloudy/Cloudy_androidKt { | ||
| public static final fun cloudy (Landroidx/compose/ui/Modifier;IZLkotlin/jvm/functions/Function1;Landroidx/compose/runtime/Composer;II)Landroidx/compose/ui/Modifier; | ||
| public static final fun cloudy (Landroidx/compose/ui/Modifier;IZLkotlin/jvm/functions/Function1;Ljava/lang/String;Landroidx/compose/runtime/Composer;II)Landroidx/compose/ui/Modifier; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "fun Modifier\.cloudy" --type=kotlin -A 10Repository: skydoves/Cloudy
Length of output: 2681
debugTag parameter has a default value in the expect declaration but is missing in platform-specific implementations.
The common expect declaration (Cloudy.kt:92) correctly includes debugTag: String = "", confirming source-level compatibility. However, the actual implementations in Cloudy.android.kt:35 and Cloudy.ios.kt:66 are missing default values for this parameter. This creates a platform-specific incompatibility where callers using the default value on the common API will fail at runtime on Android and iOS platforms.
🤖 Prompt for AI Agents
In cloudy/api/cloudy.api around line 56 (and corresponding implementations
Cloudy.android.kt:35 and Cloudy.ios.kt:66), the platform-specific function
signatures are missing the default value for the debugTag parameter; update the
Android and iOS implementation signatures to declare debugTag: String = "" to
match the common expect declaration so callers using the default compile and run
correctly; adjust any generated metadata/ABI if needed and rebuild to ensure the
default propagates to platform binaries.
| override fun update(node: CloudyModifierNode) { | ||
| node.onUpdate() | ||
| node.updateRadius(radius) | ||
| } | ||
| } |
There was a problem hiding this comment.
onStateChanged callback not updated on recomposition.
When the element is updated, only radius is propagated to the node. If the onStateChanged callback changes between compositions, the node will continue using the stale callback.
override fun update(node: CloudyModifierNode) {
node.onUpdate()
node.updateRadius(radius)
+ node.updateOnStateChanged(onStateChanged)
}Add an updateOnStateChanged method to CloudyModifierNode:
fun updateOnStateChanged(callback: (CloudyState) -> Unit) {
onStateChanged = callback
}This requires changing onStateChanged from private val to private var.
- Introduce pendingInvalidateRequest flag in CloudyModifierNode to handle concurrent draw requests efficiently, preventing blur omissions in LazyColumn scenarios. - Modify drawing logic to prioritize cached blurred bitmaps (replacing original content) to improve animated blur smoothness and resolve flickering/darkening. - Adjust updateRadius and onUpdate to correctly manage the new pendingInvalidateRequest state. - Ensure invalidateDraw() is triggered for missed requests after blur processing completes.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
Docstrings generation was requested by @l2hyunwoo. * #78 (comment) The following files were modified: * `androidMain/androidx/compose/ui/graphics/AndroidGraphicsContext.android.kt` * `androidMain/androidx/compose/ui/graphics/layer/GraphicsLayerV29.android.kt` * `app/src/androidMain/kotlin/demo/BackHandler.android.kt` * `app/src/commonMain/kotlin/demo/BackHandler.kt` * `app/src/commonMain/kotlin/demo/Main.kt` * `app/src/iosMain/kotlin/demo/BackHandler.ios.kt` * `cloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.kt` * `cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyBlurStrategy.kt` * `cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt` * `cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyRenderEffectStrategy.kt` * `cloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/TestUtils.kt` * `cloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.kt` * `cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt` * `iosApp/iosApp/ContentView.swift`
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt (3)
69-78:onStateChangedcallback not updated on recomposition (stale callback).
CloudyModifierNodeElement.updateonly forwardsradius, so if the caller passes a newonStateChangedlambda on recomposition, the node will keep invoking the old one.You can fix this by adding an update function on the node and calling it from
update:override fun update(node: CloudyModifierNode) { node.onUpdate() node.updateRadius(radius) + node.updateOnStateChanged(onStateChanged) }And in
CloudyModifierNode, switch to a mutable callback and provide the updater (example):private class CloudyModifierNode( radius: Int = 10, initialOnStateChanged: (CloudyState) -> Unit = {}, ) : Modifier.Node(), DrawModifierNode { private var onStateChanged: (CloudyState) -> Unit = initialOnStateChanged fun updateOnStateChanged(callback: (CloudyState) -> Unit) { onStateChanged = callback } // ... }
153-227: Handle bitmap capture failures and cancellation more robustly (avoid leaks & spurious errors).Two issues in the blur coroutine:
Graphics layer leak & silent failure on capture error (duplicate of prior review).
In:val capturedBitmap: Bitmap = try { graphicsLayer.toImageBitmap().asAndroidBitmap() } catch (_: Exception) { return@launch }if
toImageBitmap()throws, youreturn@launchwithout releasinggraphicsLayerand without emitting an error state. The outercatch/finallywon’t release it because no exception is propagated.Suggested fix:
val capturedBitmap: Bitmap = try { graphicsLayer.toImageBitmap().asAndroidBitmap()
- } catch (_: Exception) {
- return@launch
- }
- } catch (e: Exception) {
- if (!graphicsLayer.isReleased) {
graphicsContext.releaseGraphicsLayer(graphicsLayer)- }
- onStateChanged.invoke(
CloudyState.Error(RuntimeException("Failed to capture bitmap", e))- )
- return@launch
- }
Cancellation reported as
Error.
In the outercatch (e: Exception), ablurJob?.cancel()will surface asCloudyState.Error(e), even though cancellation is part of normal radius/content change flow. Consider treatingCancellationExceptionas non-error:} catch (e: Exception) { if (!graphicsLayer.isReleased) { graphicsContext.releaseGraphicsLayer(graphicsLayer) } if (e is kotlinx.coroutines.CancellationException) { return@launch // normal cancellation; no error state } onStateChanged.invoke(CloudyState.Error(e)) }(Add the appropriate
CancellationExceptionimport.)
234-248: Guard against division by zero inisTransparentBitmap.When
grid == 1,(grid - 1)becomes0, so therow.toFloat() / (grid - 1)andcol.toFloat() / (grid - 1)terms will divide by zero. While defaultgrid = 4is safe, this helper is more robust with an explicit guard.One simple fix is to clamp the denominator:
private fun isTransparentBitmap(bitmap: Bitmap, grid: Int = 4): Boolean { if (bitmap.width == 0 || bitmap.height == 0) return true val maxX = bitmap.width - 1 val maxY = bitmap.height - 1 + val steps = (grid - 1).coerceAtLeast(1) var nonZeroAlpha = 0 for (row in 0 until grid) { - val y = (row.toFloat() / (grid - 1) * maxY).toInt().coerceIn(0, maxY) + val y = (row.toFloat() / steps * maxY).toInt().coerceIn(0, maxY) for (col in 0 until grid) { - val x = (col.toFloat() / (grid - 1) * maxX).toInt().coerceIn(0, maxX) + val x = (col.toFloat() / steps * maxX).toInt().coerceIn(0, maxX) val pixel = bitmap.getPixel(x, y) val alpha = (pixel shr 24) and 0xFF if (alpha > 0) nonZeroAlpha++ } } return nonZeroAlpha == 0 }
🧹 Nitpick comments (1)
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt (1)
44-56: Consider wiringdebugTagthrough for diagnostics.
applyreceivesdebugTagbut drops it when constructingCloudyModifierNodeElement. IfdebugTagis meant to help trace per-instance behavior (as in the new strategy-based architecture), consider threading it into the node (and optionallyInspectorInfo) so you can correlate logs/states with specific modifiers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: l2hyunwoo
Repo: skydoves/Cloudy PR: 63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.820Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
📚 Learning: 2025-07-06T06:13:55.820Z
Learnt from: l2hyunwoo
Repo: skydoves/Cloudy PR: 63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.820Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
Applied to files:
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt
🧬 Code graph analysis (1)
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt (1)
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/internals/render/RenderScriptToolkit.kt (1)
iterativeBlur(377-404)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: API check
- GitHub Check: Spotless check
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
Docstrings generation was requested by @l2hyunwoo. * #78 (comment) The following files were modified: * `androidMain/androidx/compose/ui/graphics/AndroidGraphicsContext.android.kt` * `androidMain/androidx/compose/ui/graphics/layer/GraphicsLayerV29.android.kt` * `app/src/androidMain/kotlin/demo/BackHandler.android.kt` * `app/src/commonMain/kotlin/demo/BackHandler.kt` * `app/src/commonMain/kotlin/demo/Main.kt` * `app/src/iosMain/kotlin/demo/BackHandler.ios.kt` * `cloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.kt` * `cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyBlurStrategy.kt` * `cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt` * `cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyRenderEffectStrategy.kt` * `cloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/TestUtils.kt` * `cloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.kt` * `cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt` * `iosApp/iosApp/ContentView.swift`
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt (1)
52-52:debugTagparameter is declared but unused.The
debugTagparameter is part of the function signature but never referenced in the implementation. Consider either:
- Using it for logging/debugging (e.g., in a debug build)
- Adding a suppress annotation if intentionally unused
@Composable public actual fun Modifier.cloudy( @IntRange(from = 0) radius: Int, enabled: Boolean, onStateChanged: (CloudyState) -> Unit, + @Suppress("UNUSED_PARAMETER") debugTag: String, ): Modifier {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.kt(1 hunks)cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: l2hyunwoo
Repo: skydoves/Cloudy PR: 63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.820Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
📚 Learning: 2025-07-06T06:13:55.820Z
Learnt from: l2hyunwoo
Repo: skydoves/Cloudy PR: 63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.820Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
Applied to files:
cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: Spotless check
- GitHub Check: API check
🔇 Additional comments (1)
cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt (1)
60-80: LGTM! State signaling and blur implementation are correct.Good improvements:
LaunchedEffect(radius)properly addresses the side-effect concern from the previous review by moving the callback out of the composition phase.- The sigma conversion (
radius / 2.0f) andBlurEffectapplication are consistent with the documented behavior.TileMode.Clampis an appropriate choice for edge treatment.
Docstrings generation was requested by @l2hyunwoo. * #78 (comment) The following files were modified: * `androidMain/androidx/compose/ui/graphics/AndroidGraphicsContext.android.kt` * `androidMain/androidx/compose/ui/graphics/layer/GraphicsLayerV29.android.kt` * `app/src/androidMain/kotlin/demo/BackHandler.android.kt` * `app/src/commonMain/kotlin/demo/BackHandler.kt` * `app/src/commonMain/kotlin/demo/Main.kt` * `app/src/iosMain/kotlin/demo/BackHandler.ios.kt` * `cloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.kt` * `cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyBlurStrategy.kt` * `cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt` * `cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyRenderEffectStrategy.kt` * `cloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/TestUtils.kt` * `cloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.kt` * `cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt` * `iosApp/iosApp/ContentView.swift` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt (3)
99-102:onStateChangedcallback not updated on recomposition.When the element is updated, only
radiusis propagated to the node viaupdateRadius(). If theonStateChangedcallback changes between compositions, the node will continue using the stale callback.Add an
updateOnStateChangedmethod toCloudyModifierNodeand call it fromupdate():override fun update(node: CloudyModifierNode) { node.onUpdate() node.updateRadius(radius) + node.updateOnStateChanged(onStateChanged) }And modify
CloudyModifierNode:private var onStateChanged: (CloudyState) -> Unit = {} fun updateOnStateChanged(callback: (CloudyState) -> Unit) { onStateChanged = callback }
209-213: Resource leak and silent failure on bitmap capture exception.When
graphicsLayer.toImageBitmap()throws, the method returns early without releasinggraphicsLayeror notifying the error state. This causes a resource leak and leaves the caller unaware of the failure.val capturedBitmap: Bitmap = try { graphicsLayer.toImageBitmap().asAndroidBitmap() } catch (_: Exception) { + graphicsContext.releaseGraphicsLayer(graphicsLayer) + onStateChanged.invoke(CloudyState.Error(RuntimeException("Failed to capture bitmap"))) return@launch }
292-308: Potential division by zero whengridequals 1.If
gridis 1, the expressionrow.toFloat() / (grid - 1)at line 299 divides by zero. While the default value of 4 is safe and the function is private, adding a guard improves robustness.private fun isTransparentBitmap(bitmap: Bitmap, grid: Int = 4): Boolean { if (bitmap.width == 0 || bitmap.height == 0) return true + if (grid < 2) return false // Need at least 2x2 grid for sampling val maxX = bitmap.width - 1cloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.kt (1)
77-88: Syntax error: Missing KDoc closing delimiter.Line 87 ends the
@returndocumentation but the KDoc block is never closed with*/before the@Composableannotation on line 88. This will cause a compilation error.Additionally, there are still two consecutive KDoc blocks for the same function. The first block (lines 21-76) is comprehensive but lacks
debugTagdocumentation, while this second block documentsdebugTagbut is redundant. Consider merging thedebugTagdocumentation into the first block and removing this duplicate.* @param debugTag Optional tag appended to internal debug traces for identifying this modifier instance. * @return A Modifier with the requested blur effect applied (or the original modifier when disabled or unsupported). + */ @Composable public expect fun Modifier.cloudy(cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt (1)
26-56: Remove duplicate KDoc blocks.Two separate KDoc blocks are present (lines 26-46 and 47-56), despite a previous review marking this as addressed. The first block is more comprehensive but missing
@param debugTagand@throwsdocumentation, while the second includes these but duplicates content.Consolidate into a single KDoc block that includes all information:
/** * iOS implementation of the cloudy modifier that applies blur effects to composables. * * This implementation uses Skia's GPU-accelerated blur via [BlurEffect] which is backed * by Metal on iOS for optimal performance. The blur is applied directly in the rendering * pipeline without bitmap extraction. * * ## Performance * - Uses GPU acceleration via Skia's Metal backend * - No bitmap extraction (GPU→CPU readback) for maximum performance * - Returns [CloudyState.Success.Applied] to indicate GPU blur was applied * * ## Sigma Conversion * The blur radius is converted to sigma using: `sigma = radius / 2.0` * * @param radius The blur radius in pixels. Higher values create more blur. * @param enabled Whether the blur effect is enabled. When false, returns the original modifier unchanged. * @param onStateChanged Callback that receives updates about the blur processing state. * On iOS, this will receive [CloudyState.Success.Applied] (no bitmap available). + * @param debugTag Optional tag for debugging purposes (currently unused on iOS for GPU path). * @return Modified Modifier with blur effect applied. + * @throws IllegalArgumentException if [radius] is negative. */ -/** - * Applies an iOS GPU-accelerated blur to this Modifier when enabled and radius is greater than zero. - * - * @param radius The blur radius (must be >= 0). - * @param enabled If `false`, the original Modifier is returned unchanged. - * @param onStateChanged Callback invoked with `CloudyState.Success.Applied` when the blur radius changes and the effect is applied. - * @param debugTag Unused debug tag available for callers. - * @return A Modifier with a GPU-accelerated blur renderEffect applied when `enabled` is `true` and `radius` > 0; otherwise the original Modifier. - * @throws IllegalArgumentException if `radius` is negative. - */ @Composable public actual fun Modifier.cloudy(
🧹 Nitpick comments (8)
iosApp/iosApp/ContentView.swift (1)
6-8: Consider consolidating redundant documentation.Lines 6 and 7 convey essentially the same information. Consider keeping only one of them for brevity.
Apply this diff to streamline the documentation:
- /// Creates the view controller used by this UIViewControllerRepresentable. /// Creates the UIViewController that will be presented by this UIViewControllerRepresentable. /// - Returns: An instance of `MainViewControllerKt.MainViewController`.cloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/TestUtils.kt (1)
22-35: Remove duplicate KDoc blocks.There are two consecutive KDoc blocks documenting the same function. Keep only one (preferably the more detailed second block at lines 29-35) and remove the first.
-/** - * Creates a mock `PlatformBitmap` of the specified width and height for testing purposes. - * - * @param width The width of the bitmap in pixels. Defaults to 100. - * @param height The height of the bitmap in pixels. Defaults to 100. - * @return A `PlatformBitmap` instance wrapping a mocked Android Bitmap. - */ /** * Creates a test PlatformBitmap that wraps a Mockito-mocked Android Bitmap with the given dimensions and a non-recycled, mutable state. *androidMain/androidx/compose/ui/graphics/layer/GraphicsLayerV29.android.kt (1)
347-371: Inconsistent indentation in private helper methods.The methods
obtainLayerPaint(),requiresCompositingLayer(), andrequiresLayerPaint()have extra indentation compared to the rest of the class. This appears to be a formatting issue.- /** - * Gets the Paint instance used for the layer, creating and caching it on first access. - * - * @return The cached `android.graphics.Paint` used to apply layer compositing properties. */ - private fun obtainLayerPaint(): android.graphics.Paint = - layerPaint ?: android.graphics.Paint().also { layerPaint = it } + /** + * Gets the Paint instance used for the layer, creating and caching it on first access. + * + * @return The cached `android.graphics.Paint` used to apply layer compositing properties. + */ + private fun obtainLayerPaint(): android.graphics.Paint = + layerPaint ?: android.graphics.Paint().also { layerPaint = it } - /** - * Determines whether this graphics layer requires a separate compositing layer. - * - * @return `true` if the compositing strategy is `Offscreen`, a layer paint is required, or a render effect is present; `false` otherwise. - */ - private fun requiresCompositingLayer(): Boolean = - compositingStrategy == CompositingStrategy.Offscreen || - requiresLayerPaint() || - renderEffect != null + /** + * Determines whether this graphics layer requires a separate compositing layer. + * + * @return `true` if the compositing strategy is `Offscreen`, a layer paint is required, or a render effect is present; `false` otherwise. + */ + private fun requiresCompositingLayer(): Boolean = + compositingStrategy == CompositingStrategy.Offscreen || + requiresLayerPaint() || + renderEffect != null - /** - * Indicates whether the layer requires a dedicated Paint for compositing. - * - * @return `true` if the layer needs a Paint because the blend mode is not `SrcOver` or a color filter is set, `false` otherwise. - */ - private fun requiresLayerPaint(): Boolean = - blendMode != BlendMode.SrcOver || colorFilter != null + /** + * Indicates whether the layer requires a dedicated Paint for compositing. + * + * @return `true` if the layer needs a Paint because the blend mode is not `SrcOver` or a color filter is set, `false` otherwise. + */ + private fun requiresLayerPaint(): Boolean = + blendMode != BlendMode.SrcOver || colorFilter != nullcloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt (1)
233-241: Consider handling potentialOutOfMemoryErrorduring blur processing.The
iterativeBluroperation creates intermediate bitmaps and could fail withOutOfMemoryErroron memory-constrained devices, especially with large images. The currentcatch (e: Exception)at line 259 won't catch this.- } catch (e: Exception) { + } catch (e: Throwable) { if (!graphicsLayer.isReleased) { graphicsContext.releaseGraphicsLayer(graphicsLayer) } - onStateChanged.invoke(CloudyState.Error(e)) + onStateChanged.invoke(CloudyState.Error(if (e is Exception) e else RuntimeException(e))) } finally {Alternatively, wrap bitmap allocation in a try-catch to handle OOM gracefully.
cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt (1)
62-62: Clarify the unuseddebugTagparameter.The
debugTagparameter is accepted but never used in the iOS implementation. While this is likely intentional for cross-platform API consistency (Android strategies use it), consider documenting this explicitly in the KDoc.Update the
@param debugTagdocumentation to clarify:* @param debugTag Reserved for debugging purposes; currently unused in iOS GPU-accelerated path.app/src/commonMain/kotlin/demo/Main.kt (3)
111-146: Radius list screen structure is clean; consider adding item keys if this grows
testRadiusListplusRadiusListScreenmake the demo intent very readable, and padding/insets handling is solid. If this screen later evolves into a dynamic list, consider adding a stable key toitems(testRadiusList)(e.g.,key = { it }) to guard against item reordering/state mismatches.
208-271: BlurDetailScreen layout and back handling are well-structuredUsing
PlatformBackHandleralongside a top app bar back affordance gives a predictable detail experience, and the scrollable column that composes static, animated, and text blur tests is easy to follow. If you want to polish accessibility further, you could replace the plain "←" text button with an icon (or text) that includes a descriptive label for screen readers (e.g., “Back”).
273-342: Replayable blur animation in BlurTestCard with radius/flag changesThe separation between static and animated cards and the use of
animateIntAsStateare solid. One small enhancement: ifBlurTestCardis ever reused with a changingradiusoranimatedflag (beyond the current fixed two-card usage), the animation will only run on the first composition because bothrememberandLaunchedEffectare keyed toUnit.To make the component robust to prop changes (e.g., different radii over time), you can key both the state and the side‑effect on
radiusandanimated:- var animationPlayed by remember { mutableStateOf(!animated) } + var animationPlayed by remember(radius, animated) { mutableStateOf(!animated) } @@ - LaunchedEffect(Unit) { + LaunchedEffect(radius, animated) { if (animated) { animationPlayed = true } }This keeps current behavior for the demo but ensures the blur animates correctly if the inputs change dynamically.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
androidMain/androidx/compose/ui/graphics/AndroidGraphicsContext.android.kt(1 hunks)androidMain/androidx/compose/ui/graphics/layer/GraphicsLayerV29.android.kt(1 hunks)app/src/androidMain/kotlin/demo/BackHandler.android.kt(1 hunks)app/src/commonMain/kotlin/demo/BackHandler.kt(1 hunks)app/src/commonMain/kotlin/demo/Main.kt(2 hunks)app/src/iosMain/kotlin/demo/BackHandler.ios.kt(1 hunks)cloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.kt(1 hunks)cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyBlurStrategy.kt(1 hunks)cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt(1 hunks)cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyRenderEffectStrategy.kt(1 hunks)cloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/TestUtils.kt(1 hunks)cloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.kt(1 hunks)cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt(1 hunks)iosApp/iosApp/ContentView.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyRenderEffectStrategy.kt
- androidMain/androidx/compose/ui/graphics/AndroidGraphicsContext.android.kt
- app/src/commonMain/kotlin/demo/BackHandler.kt
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: l2hyunwoo
Repo: skydoves/Cloudy PR: 63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.820Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
📚 Learning: 2025-07-06T06:13:55.820Z
Learnt from: l2hyunwoo
Repo: skydoves/Cloudy PR: 63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.820Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
Applied to files:
cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.ktcloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/TestUtils.kt
🧬 Code graph analysis (1)
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt (1)
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/internals/render/RenderScriptToolkit.kt (1)
iterativeBlur(377-404)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: API check
- GitHub Check: Spotless check
🔇 Additional comments (12)
iosApp/iosApp/ContentView.swift (1)
3-3: LGTM! Import aligns with framework rename.The change from
ComposeApptoCloudyAppis consistent with the framework renaming across the PR.app/src/iosMain/kotlin/demo/BackHandler.ios.kt (1)
20-29: LGTM for the iOS no-op back handlerKDoc and implementation are aligned: providing a no-op
PlatformBackHandleron iOS is a clear and safe way to satisfy the common expect API where there is no system back button.app/src/androidMain/kotlin/demo/BackHandler.android.kt (1)
21-29: Android BackHandler wiring is correct and properly importedThe implementation correctly delegates
PlatformBackHandlertoandroidx.activity.compose.BackHandlerwith no shadowing concerns. The import on line 18 is explicit and unambiguous, and no localBackHandlerdefinitions exist elsewhere in the codebase that would create conflicts. The KDoc accurately documents the parameters, and the pattern matches the expected Compose API for platform-specific back handling.cloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/TestUtils.kt (1)
36-43: LGTM!The mock setup correctly stubs the essential Bitmap properties needed for unit testing the blur pipeline.
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyBlurStrategy.kt (1)
27-49: LGTM!Clean strategy interface design with clear contract documentation. The separation of GPU and CPU blur strategies via this interface improves maintainability and testability.
androidMain/androidx/compose/ui/graphics/layer/GraphicsLayerV29.android.kt (1)
44-61: LGTM!The GraphicsLayerV29 implementation correctly wraps the public RenderNode API for Android Q+ with appropriate initialization and compositing strategy defaults.
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt (1)
170-183: LGTM!The early exit for zero/negative radius correctly bypasses blur processing and reports
Success.Appliedstate.cloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.kt (1)
26-74: LGTM! Clean refactoring to strategy pattern.The strategy-based approach cleanly separates concerns and handles all edge cases appropriately. The API-level branching is correct, and the KDoc is comprehensive.
cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt (1)
70-90: LGTM! Proper side effect management with LaunchedEffect.The use of
LaunchedEffect(radius)correctly addresses the previous review concern about unconditional side effects during composition. The state notification is properly scoped to radius changes, and the GPU blur application viaBlurEffectis clean and efficient.app/src/commonMain/kotlin/demo/Main.kt (3)
74-108: CloudyDemoApp navigation and AnimatedContent usage look cohesiveThe list/detail navigation via
selectedRadiusandAnimatedContentis clear, and the KDoc accurately describes the flow and transitions. Nice use of directional slide + fade to communicate navigation context.
148-206: RadiusListItem composition and blur preview look goodThe card layout, preview image with
.cloudy(radius = radius, debugTag = "RadiusListItem"), and the sigma text (radius / 2.0f) align well with the new blur engine semantics. The "No blur" special‑case at radius 0 is also a nice UX touch.
344-404: TextBlurTest nicely showcases content blurThe blurred column around the poster title/description is a clear demonstration of text/content blur, and the KDoc matches the behavior. The use of a dedicated
debugTag("TextBlurColumn") will also help when inspecting engine behavior.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
151-163: Clarify platform-specific behavior in the "Maintaining Blurring Effect" section.Line 153 states the modifier "captures the bitmap of the composable node under the hood," but this is only true for the CPU path (Android 30-). On GPU paths (iOS and Android 31+), no bitmap is captured—the blur is rendered directly. The example context (LazyVerticalGrid) should either specify the target platform or clarify that bitmap capture is platform-dependent.
Update the comment to reflect platform-specific behavior:
-The `Modifier.cloudy` captures the bitmap of the composable node under the hood. +The `Modifier.cloudy` behavior depends on the platform: on Android 30-, it captures a bitmap on a background thread; on iOS and Android 31+, blur is rendered directly without bitmap extraction. See the Platform Support table for details.
♻️ Duplicate comments (4)
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyRenderEffectStrategy.kt (2)
57-63: Redundant API check insidegraphicsLayer.The strategy selection in
Cloudy.android.ktalready gates this to API 31+. The check is redundant but provides defense-in-depth. Consider adding@RequiresApi(Build.VERSION_CODES.S)to the class instead, which would allow removing the runtime check.+import androidx.annotation.RequiresApi + +@RequiresApi(Build.VERSION_CODES.S) internal object CloudyRenderEffectStrategy : CloudyBlurStrategy { // ... return modifier.graphicsLayer { - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { - renderEffect = RenderEffect - .createBlurEffect(sigma, sigma, Shader.TileMode.CLAMP) - .asComposeRenderEffect() - } + renderEffect = RenderEffect + .createBlurEffect(sigma, sigma, Shader.TileMode.CLAMP) + .asComposeRenderEffect() }
47-53:Success.Appliedemitted even whenradius=0.The
LaunchedEffect(radius)emitsSuccess.Appliedbefore theradius == 0check. When blur is effectively disabled, the state still reportsSuccess.Applied, which may be misleading.@Composable override fun apply( modifier: Modifier, radius: Int, onStateChanged: (CloudyState) -> Unit, ): Modifier { + if (radius == 0) { + LaunchedEffect(Unit) { + onStateChanged(CloudyState.Nothing) + } + return modifier + } + LaunchedEffect(radius) { onStateChanged(CloudyState.Success.Applied) } - - if (radius == 0) { - return modifier - } val sigma = radius / 2.0fcloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt (2)
98-101:onStateChangedcallback not updated on recomposition.When the element is updated, only
radiusis propagated to the node. If theonStateChangedcallback changes between compositions, the node continues using the stale callback.override fun update(node: CloudyModifierNode) { node.onUpdate() node.updateRadius(radius) + node.updateOnStateChanged(onStateChanged) }This requires adding an
updateOnStateChangedmethod toCloudyModifierNodeand changingonStateChangedfromprivate valtoprivate var.
280-296: Edge case: potential division by zero whengridis 1.If
gridis 1, the expressionrow.toFloat() / (grid - 1)divides by zero. While the default is 4 and the function is private, consider adding a guard.private fun isTransparentBitmap(bitmap: Bitmap, grid: Int = 4): Boolean { if (bitmap.width == 0 || bitmap.height == 0) return true + if (grid < 2) return false // Need at least 2x2 grid for sampling val maxX = bitmap.width - 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
README.md(2 hunks)app/src/androidMain/kotlin/demo/BackHandler.android.kt(1 hunks)app/src/commonMain/kotlin/demo/BackHandler.kt(1 hunks)app/src/commonMain/kotlin/demo/Main.kt(2 hunks)app/src/iosMain/kotlin/demo/BackHandler.ios.kt(1 hunks)cloudy/api/cloudy.api(1 hunks)cloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.kt(2 hunks)cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyBlurStrategy.kt(1 hunks)cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt(1 hunks)cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyRenderEffectStrategy.kt(1 hunks)cloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/TestUtils.kt(1 hunks)cloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.kt(1 hunks)cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/androidMain/kotlin/demo/BackHandler.android.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/Cloudy.kt
- app/src/commonMain/kotlin/demo/Main.kt
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: l2hyunwoo
Repo: skydoves/Cloudy PR: 63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.820Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
📚 Learning: 2025-07-06T06:13:55.820Z
Learnt from: l2hyunwoo
Repo: skydoves/Cloudy PR: 63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.820Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
Applied to files:
README.mdcloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.ktcloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyRenderEffectStrategy.ktcloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/TestUtils.ktcloudy/api/cloudy.api
🧬 Code graph analysis (1)
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt (1)
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/internals/render/RenderScriptToolkit.kt (1)
iterativeBlur(377-404)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: Spotless check
- GitHub Check: API check
🔇 Additional comments (9)
README.md (1)
5-5: Documentation accurately reflects the new GPU-first architecture and state redesign. ✓The updated intro, CloudyState tables, platform support matrix, and code examples correctly document the blur engine overhaul. The when-expression pattern over state subtypes (
Applied,Captured,Loading,Error,Nothing) clearly shows the new state-driven API. Platform distinctions (GPU vs. CPU) are explicit and helpful.Also applies to: 13-13, 17-17, 106-130, 133-150
app/src/commonMain/kotlin/demo/BackHandler.kt (1)
1-27: LGTM! Well-structured multiplatform API.The expect declaration follows Kotlin multiplatform conventions correctly. The KDoc is clear, and the default parameter value placement is appropriate for the expect side.
app/src/iosMain/kotlin/demo/BackHandler.ios.kt (1)
1-29: LGTM! Appropriate no-op implementation for iOS.The actual implementation correctly omits the default parameter value (inherited from the expect declaration) and appropriately provides a no-op implementation since iOS uses swipe gestures instead of a system back button. The KDoc clearly explains the platform-specific behavior.
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyBlurStrategy.kt (1)
27-42: Clean strategy interface design.The interface provides a clear contract for platform-specific blur implementations. The
@Composableannotation onapplyallows strategies to use Compose effects internally, which is appropriate for the RenderEffect strategy'sLaunchedEffectusage.cloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/TestUtils.kt (1)
36-43: LGTM!The mock setup correctly stubs the essential
Bitmapproperties needed for testing the blur strategies.cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt (2)
43-64: LGTM!The strategy object correctly delegates to
CloudyModifierNodeElementfor the legacy blur path.
107-145: LGTM!The node's state management properly handles radius updates by canceling ongoing blur jobs and the
pendingInvalidateRequestflag correctly addresses race conditions mentioned in the PR objectives.cloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.kt (1)
45-55: Clean strategy-based architecture.The API-level branching is clear and the delegation to strategy implementations is straightforward. This design cleanly separates the GPU and CPU blur paths.
cloudy/api/cloudy.api (1)
32-53: LGTM! CloudyState.Success refactoring is well-structured.The changes correctly implement the sealed-interface pattern for
CloudyState.Success:
- Success as interface (lines 32-33): Allows polymorphic handling of Applied and Captured variants
- Applied singleton (lines 35-41): GPU-accelerated path with no bitmap extraction, appropriate for render-effect-based blur
- Captured with bitmap (lines 43-53): Legacy CPU path providing bitmap access
The copy methods on
Capturedcorrectly return the concrete typeCloudyState$Success$Captured(lines 47-48) rather than the interface, which maintains type safety and follows Kotlin data class conventions.This refactoring aligns perfectly with the PR's GPU-first architecture and provides a clean API for distinguishing between GPU-applied blur (no bitmap) and CPU-captured blur (with bitmap).
| /** | ||
| * Creates a mock `PlatformBitmap` of the specified width and height for testing purposes. | ||
| * | ||
| * @param width The width of the bitmap in pixels. Defaults to 100. | ||
| * @param height The height of the bitmap in pixels. Defaults to 100. | ||
| * @return A `PlatformBitmap` instance wrapping a mocked Android Bitmap. | ||
| */ | ||
| /** | ||
| * Creates a test PlatformBitmap that wraps a Mockito-mocked Android Bitmap with the given dimensions and a non-recycled, mutable state. | ||
| * | ||
| * @param width The mocked bitmap width in pixels. | ||
| * @param height The mocked bitmap height in pixels. | ||
| * @return A PlatformBitmap containing the configured mock Bitmap. | ||
| */ |
There was a problem hiding this comment.
Remove duplicate KDoc block.
There are two consecutive KDoc blocks for the same function. Keep only one.
-/**
- * Creates a mock `PlatformBitmap` of the specified width and height for testing purposes.
- *
- * @param width The width of the bitmap in pixels. Defaults to 100.
- * @param height The height of the bitmap in pixels. Defaults to 100.
- * @return A `PlatformBitmap` instance wrapping a mocked Android Bitmap.
- */
/**
* Creates a test PlatformBitmap that wraps a Mockito-mocked Android Bitmap with the given dimensions and a non-recycled, mutable state.
*
* @param width The mocked bitmap width in pixels.
* @param height The mocked bitmap height in pixels.
* @return A PlatformBitmap containing the configured mock Bitmap.
*/
internal fun createMockPlatformBitmap(width: Int = 100, height: Int = 100): PlatformBitmap {🤖 Prompt for AI Agents
In cloudy/src/androidUnitTest/kotlin/com/skydoves/cloudy/TestUtils.kt around
lines 22 to 35, there are two consecutive KDoc blocks documenting the same
function; remove the duplicate block so only a single KDoc comment remains
directly above the function declaration, preserving the more complete/desired
documentation text and leaving surrounding code and formatting unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt (1)
98-102:onStateChangedcallback not updated on recomposition.When the element is updated, only
radiusis propagated to the node. If theonStateChangedcallback changes between compositions, the node will continue using the stale callback.override fun update(node: CloudyModifierNode) { node.onUpdate() node.updateRadius(radius) + node.updateOnStateChanged(onStateChanged) }Add an
updateOnStateChangedmethod toCloudyModifierNodeand changeonStateChangedfromprivate valtoprivate var:private var onStateChanged: (CloudyState) -> Unit = {} fun updateOnStateChanged(callback: (CloudyState) -> Unit) { onStateChanged = callback }
🧹 Nitpick comments (1)
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt (1)
194-227: Consider recycling intermediate bitmaps for memory optimization.On legacy devices (API 30 and below), memory is more constrained. The intermediate bitmaps (
capturedBitmapand potentially the copiedsoftwareBitmap) are not explicitly recycled after the blur completes. While they'll eventually be garbage collected, explicit recycling could help reduce memory pressure during rapid recomposition.val blurResult = withContext(Dispatchers.Default) { val outputBitmap: Bitmap = softwareBitmap.toPlatformBitmap().createCompatible().toAndroidBitmap() iterativeBlur( androidBitmap = softwareBitmap, outputBitmap = outputBitmap, radius = currentRadius, ).await() } + + // Recycle intermediate bitmaps if they differ from the result + if (softwareBitmap !== capturedBitmap && !softwareBitmap.isRecycled) { + softwareBitmap.recycle() + } + if (!capturedBitmap.isRecycled) { + capturedBitmap.recycle() + } val result = blurResult?.toPlatformBitmap()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt(1 hunks)cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyRenderEffectStrategy.kt(1 hunks)cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyRenderEffectStrategy.kt
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: l2hyunwoo
Repo: skydoves/Cloudy PR: 63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.820Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
📚 Learning: 2025-07-06T06:13:55.820Z
Learnt from: l2hyunwoo
Repo: skydoves/Cloudy PR: 63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.820Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
Applied to files:
cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.ktcloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: API check
🔇 Additional comments (7)
cloudy/src/iosMain/kotlin/com/skydoves/cloudy/Cloudy.ios.kt (1)
26-63: GPU blur path and state signaling look correct and aligned with the new model.
LaunchedEffect(radius)confinesonStateChangedside effects properly, theradius > 0guard prevents emittingSuccess.Appliedwhen no blur is rendered, and theenabled/radius == 0early returns keep the modifier side-effect free in those cases. The sigma conversion andBlurEffectconfiguration (symmetric radii,TileMode.Clamp) match the documented design of the GPU Applied path. I don’t see any correctness or lifecycle issues here.cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt (6)
39-64: Well-structured strategy pattern implementation.The
CloudyLegacyBlurStrategyobject cleanly implements the blur strategy interface with proper documentation. The@SuppressLintannotation is appropriate since the modifier receiver is used via.then().
107-121: Well-designed state management for async blur operations.The flags (
isProcessing,pendingInvalidateRequest,contentMayHaveChanged) properly coordinate between UI updates and background blur processing, addressing the race conditions mentioned in the PR objectives.
123-136: Proper state cleanup on radius change.The method correctly cancels any in-flight blur work, resets processing flags, and schedules a redraw. The
isAttachedguard beforeinvalidateDraw()is good defensive programming.
192-260: Solid error handling and resource cleanup.The structured error handling properly reports
CloudyState.Erroron failures, and thefinallyblock ensuresgraphicsLayeris always released. CancellationException is correctly rethrown to support coroutine cancellation.
233-244: Verify transparent bitmap behavior aligns with expected UX.When the blurred result is fully transparent (lines 235-237), the node sets
contentMayHaveChanged = trueand clearsisProcessing, but no state callback is invoked—the caller remains inLoading. This appears intentional for passive waiting on async image loaders per PR objectives, but if content is genuinely transparent (e.g., a fully transparent view), the caller will stay inLoadingindefinitely.Consider whether an explicit state (e.g.,
Loadingwith a hint or a new state variant) should be emitted to inform callers that content capture yielded an empty result.
278-295: Division by zero guard correctly implemented.The guard at line 280 (
if (grid <= 1) return false) prevents the division-by-zero issue identified in a previous review. The grid-based sampling approach is efficient for detecting empty captures.
1 participant
🎯 Goal
Improve blur performance with platform-optimized GPU implementations and stabilize legacy CPU rendering
This PR introduces a major architectural upgrade to the blur engine:
ImageFilter.makeBlur()for GPU-accelerated blur via Metal backend (replacing placeholder bitmaps).RenderEffectfor zero-copy, GPU-accelerated blur on the RenderThread.Breaking Change:
CloudyState.Successis now a sealed interface withSuccess.AppliedandSuccess.Capturedsubtypes.🛠 Implementation details
1. CloudyState Redesign
Separated the success state to distinguish between GPU (direct rendering) and CPU (bitmap capture) paths.
2. Platform Strategies
Success.AppliedSuccess.CapturedSuccess.Applied3. Critical Fixes for Android API 30- (CPU Path)
Recent investigations revealed that the legacy CPU path suffered from race conditions and timing issues.
pendingInvalidateRequestflag to handle race conditions in scrolling lists (e.g.,LazyColumn). This ensures blur requests triggered during processing (e.g., by Coil loading completion) are not lost.invalidateDraw().✍️ Migration Guide & Examples
Migration for CloudyState users:
Android API Branching Logic:
Key Decisions
sigma = radius / 2.0standard formula applied consistently across all platforms.Screenshots
Screen_recording_20251205_145050.mp4
Screen_recording_20251205_145026.mp4
Summary by CodeRabbit
New Features
Updates
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.