fix: optimize blur animation handling for full and no blur states#110
fix: optimize blur animation handling for full and no blur states#110sbaiahmed1 wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThis PR removes Configuration-driven dark-mode branching from Android blur resolution and initialization, moves Android blur init to onAttachedToWindow and tightens cleanup, and adds iOS helpers and view updates to apply explicit overrideUserInterfaceStyle based on blur type strings; several iOS view simplifications and blur-intensity optimizations were also made. ChangesAndroid Configuration-Independent Blur
iOS Explicit Interface Style Overrides
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 (2)
android/src/main/java/com/sbaiahmed1/reactnativeblur/ReactNativeBlurView.kt (2)
199-209:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnregister the swapped
OnPreDrawListenerinReactNativeBlurView.cleanup()
swapBlurRootToScreenAncestor()removes the listener from the old root and re-registers it on the new screen/root ancestor, butReactNativeBlurView.cleanup()only resets flags/callbacks and never callsremoveOnPreDrawListener. SinceReactNativeBlurViewManager.onDropViewInstance()also callsview.cleanup(), dropped/detached views can leave the listener registered on the ancestorViewTreeObserver, keeping callbacks alive past teardown. Mirror the listener-unregister logic used inReactNativeProgressiveBlurView.cleanup().🤖 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 `@android/src/main/java/com/sbaiahmed1/reactnativeblur/ReactNativeBlurView.kt` around lines 199 - 209, ReactNativeBlurView.cleanup() currently only clears flags/callbacks but does not unregister the OnPreDrawListener, which can leave the listener attached to the ancestor ViewTreeObserver after teardown; update cleanup() to mirror ReactNativeProgressiveBlurView.cleanup() by checking the stored blur root/screen ancestor and calling its viewTreeObserver.removeOnPreDrawListener(blurPreDrawListener) (or the actual listener field name used), then nulling any root/ancestor references, before resetting isBlurInitialized and initRunnable; ensure swapBlurRootToScreenAncestor()'s listener registration and the listener field (e.g., blurPreDrawListener) are used consistently.
252-268:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix missing
setGlassTintColorKDoc/function block (breaks Kotlin parsing)In
ReactNativeBlurView.kt, the KDoc opened at line 252 is never closed until the*/at line 440, so thecolor?.let { ... }block (lines 254-268) and the following methods are effectively commented out;setGlassTintColor(...)also doesn’t exist elsewhere in the file.Suggested fix
/** * Set the glass tint color for liquid glass effect. + */ + fun setGlassTintColor(color: String?) { color?.let { try { glassTintColor = it.toColorInt() logDebug("setGlassTintColor: $color -> $glassTintColor") updateGlassEffect() } catch (e: Exception) { logWarning("Invalid color format for glass tint: $color") glassTintColor = Color.TRANSPARENT } } ?: run { glassTintColor = Color.TRANSPARENT logDebug("Cleared glass tint color") updateGlassEffect() } + }🤖 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 `@android/src/main/java/com/sbaiahmed1/reactnativeblur/ReactNativeBlurView.kt` around lines 252 - 268, Close the stray KDoc block before the method and restore/implement the missing setGlassTintColor function using the existing color handling logic: add a proper function signature named setGlassTintColor(...) containing the color?.let { try { glassTintColor = it.toColorInt(); logDebug("setGlassTintColor: $color -> $glassTintColor"); updateGlassEffect() } catch (e: Exception) { logWarning("Invalid color format for glass tint: $color"); glassTintColor = Color.TRANSPARENT } } ?: run { glassTintColor = Color.TRANSPARENT; logDebug("Cleared glass tint color"); updateGlassEffect() }, ensuring glassTintColor, updateGlassEffect(), logDebug(), and logWarning() are referenced exactly as in the diff and any required imports (e.g., Color, toColorInt) are present.
🧹 Nitpick comments (2)
ios/Views/VibrancyEffectView.swift (1)
98-122: ⚡ Quick winRemove duplicate style mapping logic.
The
styleFromStringmethod duplicates theblurStyleFromStringfunction inBlurStyleHelpers.swift. Prefer reusing the existing helper to maintain a single source of truth for blur style mappings.♻️ Proposed refactor to use shared helper
Remove the local
styleFromStringmethod and update line 68 to use the shared helper:- let style = styleFromString(blurType) + let style = blurStyleFromString(blurType) let blurEffect = UIBlurEffect(style: style)Then delete the duplicate method:
- // Helper to parse string to UIBlurEffect.Style - private func styleFromString(_ style: String) -> UIBlurEffect.Style { - switch style { - case "xlight": return .extraLight - case "light": return .light - case "dark": return .dark - case "regular": return .regular - case "prominent": return .prominent - case "systemUltraThinMaterial": return .systemUltraThinMaterial - case "systemThinMaterial": return .systemThinMaterial - case "systemMaterial": return .systemMaterial - case "systemThickMaterial": return .systemThickMaterial - case "systemChromeMaterial": return .systemChromeMaterial - case "systemUltraThinMaterialLight": return .systemUltraThinMaterialLight - case "systemThinMaterialLight": return .systemThinMaterialLight - case "systemMaterialLight": return .systemMaterialLight - case "systemThickMaterialLight": return .systemThickMaterialLight - case "systemChromeMaterialLight": return .systemChromeMaterialLight - case "systemUltraThinMaterialDark": return .systemUltraThinMaterialDark - case "systemThinMaterialDark": return .systemThinMaterialDark - case "systemMaterialDark": return .systemMaterialDark - case "systemThickMaterialDark": return .systemThickMaterialDark - case "systemChromeMaterialDark": return .systemChromeMaterialDark - default: return .regular - } - }🤖 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 `@ios/Views/VibrancyEffectView.swift` around lines 98 - 122, Replace the local duplicate mapping by removing the private method styleFromString in VibrancyEffectView and instead call the shared helper blurStyleFromString (from BlurStyleHelpers.swift) wherever styleFromString was used (e.g., in VibrancyEffectView's initializer/configuration code); ensure you import or reference the helper function and adjust any parameter types if needed so the view uses the single source of truth for blur style mappings.ios/Views/BasicColoredView.swift (1)
23-23: ⚡ Quick win
reducedTransparencyFallbackColoris now a no-op for this rendering path.Line 23 always uses
regularBlurView, so the initializer parameterreducedTransparencyFallbackColorno longer affects behavior. This leaves a misleading API contract for callers still passing that value.♻️ Suggested cleanup
init(blurAmount: Double, blurStyle: UIBlurEffect.Style, - ignoreSafeArea: Bool, - reducedTransparencyFallbackColor: UIColor) { + ignoreSafeArea: Bool) { self.blurAmount = blurAmount self.blurStyle = blurStyle self.ignoreSafeArea = ignoreSafeArea self.blurIntensity = mapBlurAmountToIntensity(blurAmount) }Also update
BasicColoredView(...)call sites to removereducedTransparencyFallbackColor.🤖 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 `@ios/Views/BasicColoredView.swift` at line 23, The initializer parameter reducedTransparencyFallbackColor of BasicColoredView is unused because the code always uses regularBlurView; remove the unused parameter from the BasicColoredView init signature and any stored property or parameter handling tied to reducedTransparencyFallbackColor, update the implementation so it relies solely on regularBlurView, and update all BasicColoredView(...) call sites to stop passing reducedTransparencyFallbackColor; also remove related docs/comments and any dead branches that reference reducedTransparencyFallbackColor to keep the API and implementation consistent.
🤖 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 `@example/package.json`:
- Line 10: Restore the default iOS script to run the simulator and add a
separate device-targeted script: change the "ios" npm script to "expo run:ios"
(remove the --device flag) and add a new "ios:device" script with "expo run:ios
--device"; ensure consistency with the existing "build:ios" script and update
any documentation or README references to use "ios:device" when physical-device
testing is required.
In `@ios/Views/BlurEffectView.swift`:
- Around line 39-45: In updateBlur(style:intensity:) ensure the
UIViewPropertyAnimator is recreated when the requested UIBlurEffect(style:)
differs from the currently applied style even for 0–1 intensity updates: detect
if animator exists but its effect style doesn't match the new style (compare the
new style to a stored currentStyle or to the animator's effect), and call
setupBlur() to recreate the animator and effect instead of simply setting
existing.fractionComplete; update stored currentStyle when creating the animator
so future calls can correctly decide whether to reuse or recreate.
---
Outside diff comments:
In `@android/src/main/java/com/sbaiahmed1/reactnativeblur/ReactNativeBlurView.kt`:
- Around line 199-209: ReactNativeBlurView.cleanup() currently only clears
flags/callbacks but does not unregister the OnPreDrawListener, which can leave
the listener attached to the ancestor ViewTreeObserver after teardown; update
cleanup() to mirror ReactNativeProgressiveBlurView.cleanup() by checking the
stored blur root/screen ancestor and calling its
viewTreeObserver.removeOnPreDrawListener(blurPreDrawListener) (or the actual
listener field name used), then nulling any root/ancestor references, before
resetting isBlurInitialized and initRunnable; ensure
swapBlurRootToScreenAncestor()'s listener registration and the listener field
(e.g., blurPreDrawListener) are used consistently.
- Around line 252-268: Close the stray KDoc block before the method and
restore/implement the missing setGlassTintColor function using the existing
color handling logic: add a proper function signature named
setGlassTintColor(...) containing the color?.let { try { glassTintColor =
it.toColorInt(); logDebug("setGlassTintColor: $color -> $glassTintColor");
updateGlassEffect() } catch (e: Exception) { logWarning("Invalid color format
for glass tint: $color"); glassTintColor = Color.TRANSPARENT } } ?: run {
glassTintColor = Color.TRANSPARENT; logDebug("Cleared glass tint color");
updateGlassEffect() }, ensuring glassTintColor, updateGlassEffect(), logDebug(),
and logWarning() are referenced exactly as in the diff and any required imports
(e.g., Color, toColorInt) are present.
---
Nitpick comments:
In `@ios/Views/BasicColoredView.swift`:
- Line 23: The initializer parameter reducedTransparencyFallbackColor of
BasicColoredView is unused because the code always uses regularBlurView; remove
the unused parameter from the BasicColoredView init signature and any stored
property or parameter handling tied to reducedTransparencyFallbackColor, update
the implementation so it relies solely on regularBlurView, and update all
BasicColoredView(...) call sites to stop passing
reducedTransparencyFallbackColor; also remove related docs/comments and any dead
branches that reference reducedTransparencyFallbackColor to keep the API and
implementation consistent.
In `@ios/Views/VibrancyEffectView.swift`:
- Around line 98-122: Replace the local duplicate mapping by removing the
private method styleFromString in VibrancyEffectView and instead call the shared
helper blurStyleFromString (from BlurStyleHelpers.swift) wherever
styleFromString was used (e.g., in VibrancyEffectView's
initializer/configuration code); ensure you import or reference the helper
function and adjust any parameter types if needed so the view uses the single
source of truth for blur style mappings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a03319d2-b613-4b6d-a677-2d3e7e5d1476
📒 Files selected for processing (12)
android/src/main/java/com/sbaiahmed1/reactnativeblur/BlurType.ktandroid/src/main/java/com/sbaiahmed1/reactnativeblur/ReactNativeBlurView.ktandroid/src/main/java/com/sbaiahmed1/reactnativeblur/ReactNativeProgressiveBlurView.ktexample/package.jsonios/Helpers/BlurStyleHelpers.swiftios/Views/AdvancedBlurView.swiftios/Views/BasicColoredView.swiftios/Views/BlurEffectView.swiftios/Views/LiquidGlassContainerView.swiftios/Views/ProgressiveBlurView.swiftios/Views/VariableBlurView.swiftios/Views/VibrancyEffectView.swift
💤 Files with no reviewable changes (1)
- ios/Views/VariableBlurView.swift
| "android": "expo run:android", | ||
| "prebuild:ios": "expo prebuild --platform ios --clean", | ||
| "ios": "expo run:ios", | ||
| "ios": "expo run:ios --device", |
There was a problem hiding this comment.
Reconsider forcing physical device as the default iOS target.
Changing the ios script to require --device breaks the standard development workflow for contributors who rely on simulators. Simulators are faster, more convenient for iterative testing, and don't require physical hardware.
Additionally, this is inconsistent with the build:ios script on line 14, which explicitly targets the simulator (-sdk iphonesimulator).
If device testing is necessary for validating blur effects on real hardware, consider adding a separate script instead of replacing the default:
"ios": "expo run:ios",
"ios:device": "expo run:ios --device",📱 Proposed fix to restore simulator default and add device option
- "ios": "expo run:ios --device",
+ "ios": "expo run:ios",
+ "ios:device": "expo run:ios --device",🤖 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 `@example/package.json` at line 10, Restore the default iOS script to run the
simulator and add a separate device-targeted script: change the "ios" npm script
to "expo run:ios" (remove the --device flag) and add a new "ios:device" script
with "expo run:ios --device"; ensure consistency with the existing "build:ios"
script and update any documentation or README references to use "ios:device"
when physical-device testing is required.
…and optimizing animator logic
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/src/main/java/com/sbaiahmed1/reactnativeblur/ReactNativeBlurView.kt (1)
256-271:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
updateGlassEffect()call in catch block causes visual inconsistency.When color parsing fails at line 262-264,
glassTintColoris set toColor.TRANSPARENTbutupdateGlassEffect()is not called. In contrast, the null branch at lines 266-270 correctly callsupdateGlassEffect()after setting the same value. This leaves the UI showing the previous tint color despite the internal state being TRANSPARENT.🐛 Proposed fix to ensure consistent UI update on parse failure
} catch (e: Exception) { logWarning("Invalid color format for glass tint: $color") glassTintColor = Color.TRANSPARENT + updateGlassEffect() }🤖 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 `@android/src/main/java/com/sbaiahmed1/reactnativeblur/ReactNativeBlurView.kt` around lines 256 - 271, In setGlassTintColor, the catch block sets glassTintColor = Color.TRANSPARENT but does not call updateGlassEffect(), leaving the UI stale; modify the catch handler in the setGlassTintColor method (where the Exception is caught) to call updateGlassEffect() after setting glassTintColor and logging the warning so the visual state is updated consistently (same behavior as the null branch).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@android/src/main/java/com/sbaiahmed1/reactnativeblur/ReactNativeBlurView.kt`:
- Around line 256-271: In setGlassTintColor, the catch block sets glassTintColor
= Color.TRANSPARENT but does not call updateGlassEffect(), leaving the UI stale;
modify the catch handler in the setGlassTintColor method (where the Exception is
caught) to call updateGlassEffect() after setting glassTintColor and logging the
warning so the visual state is updated consistently (same behavior as the null
branch).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09f08aa2-cf8e-494b-b25b-f19dba84fa1a
📒 Files selected for processing (2)
android/src/main/java/com/sbaiahmed1/reactnativeblur/ReactNativeBlurView.ktios/Views/BlurEffectView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- ios/Views/BlurEffectView.swift
| REGULAR(Color.argb(35, 255, 255, 255)), | ||
| PROMINENT(Color.argb(140, 240, 240, 240)), | ||
| SYSTEM_ULTRA_THIN_MATERIAL(Color.argb(75, 240, 240, 240)), | ||
| SYSTEM_ULTRA_THIN_MATERIAL_LIGHT(Color.argb(75, 240, 240, 240)), | ||
| SYSTEM_ULTRA_THIN_MATERIAL_DARK(Color.argb(65, 40, 40, 40)), | ||
| SYSTEM_THIN_MATERIAL(Color.argb(102, 240, 240, 240)), | ||
| SYSTEM_THIN_MATERIAL_LIGHT(Color.argb(102, 240, 240, 240)), | ||
| SYSTEM_THIN_MATERIAL_DARK(Color.argb(102, 35, 35, 35)), | ||
| SYSTEM_MATERIAL(Color.argb(140, 245, 245, 245)), | ||
| SYSTEM_MATERIAL_LIGHT(Color.argb(140, 245, 245, 245)), | ||
| SYSTEM_MATERIAL_DARK(Color.argb(215, 65, 60, 60)), | ||
| SYSTEM_THICK_MATERIAL(Color.argb(210, 248, 248, 248)), | ||
| SYSTEM_THICK_MATERIAL_LIGHT(Color.argb(210, 248, 248, 248)), | ||
| SYSTEM_THICK_MATERIAL_DARK(Color.argb(160, 35, 35, 35)), | ||
| SYSTEM_CHROME_MATERIAL(Color.argb(165, 248, 248, 248)), | ||
| SYSTEM_CHROME_MATERIAL_LIGHT(Color.argb(165, 248, 248, 248)), | ||
| SYSTEM_CHROME_MATERIAL_DARK(Color.argb(100, 32, 32, 32)); | ||
|
|
||
| companion object { | ||
| /** | ||
| * Get BlurType from string, with fallback to LIGHT for unknown types. | ||
| * Uses the provided configuration to determine if dark mode is active for | ||
| * appropriate defaults. | ||
| */ | ||
| fun fromString(type: String, configuration: Configuration): BlurType { | ||
| val isDarkMode = (configuration.uiMode and Configuration.UI_MODE_NIGHT_MASK) == Configuration.UI_MODE_NIGHT_YES | ||
|
|
||
| fun fromString(type: String): BlurType { | ||
| return when (type.lowercase()) { | ||
| "xlight" -> XLIGHT | ||
| "light" -> LIGHT | ||
| "dark" -> DARK | ||
| "extradark" -> EXTRA_DARK | ||
| "regular" -> if (isDarkMode) REGULAR_DARK else REGULAR_LIGHT | ||
| "prominent" -> if (isDarkMode) PROMINENT_DARK else PROMINENT_LIGHT | ||
| "systemultrathinmaterial" -> if (isDarkMode) SYSTEM_ULTRA_THIN_MATERIAL_DARK else SYSTEM_ULTRA_THIN_MATERIAL_LIGHT | ||
| "regular" -> REGULAR | ||
| "prominent" -> PROMINENT | ||
| "systemultrathinmaterial" -> SYSTEM_ULTRA_THIN_MATERIAL | ||
| "systemultrathinmateriallight" -> SYSTEM_ULTRA_THIN_MATERIAL_LIGHT | ||
| "systemultrathinmaterialdark" -> SYSTEM_ULTRA_THIN_MATERIAL_DARK | ||
| "systemthinmaterial" -> if (isDarkMode) SYSTEM_THIN_MATERIAL_DARK else SYSTEM_THIN_MATERIAL_LIGHT | ||
| "systemthinmaterial" -> SYSTEM_THIN_MATERIAL | ||
| "systemthinmateriallight" -> SYSTEM_THIN_MATERIAL_LIGHT | ||
| "systemthinmaterialdark" -> SYSTEM_THIN_MATERIAL_DARK | ||
| "systemmaterial" -> if (isDarkMode) SYSTEM_MATERIAL_DARK else SYSTEM_MATERIAL_LIGHT | ||
| "systemmaterial" -> SYSTEM_MATERIAL | ||
| "systemmateriallight" -> SYSTEM_MATERIAL_LIGHT | ||
| "systemmaterialdark" -> SYSTEM_MATERIAL_DARK | ||
| "systemthickmaterial" -> if (isDarkMode) SYSTEM_THICK_MATERIAL_DARK else SYSTEM_THICK_MATERIAL_LIGHT | ||
| "systemthickmaterial" -> SYSTEM_THICK_MATERIAL | ||
| "systemthickmateriallight" -> SYSTEM_THICK_MATERIAL_LIGHT | ||
| "systemthickmaterialdark" -> SYSTEM_THICK_MATERIAL_DARK | ||
| "systemchromematerial" -> if (isDarkMode) SYSTEM_CHROME_MATERIAL_DARK else SYSTEM_CHROME_MATERIAL_LIGHT | ||
| "systemchromematerial" -> SYSTEM_CHROME_MATERIAL | ||
| "systemchromemateriallight" -> SYSTEM_CHROME_MATERIAL_LIGHT | ||
| "systemchromematerialdark" -> SYSTEM_CHROME_MATERIAL_DARK | ||
| else -> XLIGHT // default fallback | ||
| else -> XLIGHT |
There was a problem hiding this comment.
Perhaps I didn't quite understand when we talked about the X. But why the option to adapt the color from the device's UI settings?
Could you explain to me exactly what the problem is?
| /// Determines the fixed interface style for a blur type to prevent system adaptation. | ||
| /// Returns nil for ambiguous styles that should inherit from the system. | ||
| func interfaceStyleForBlurType(_ styleString: String) -> UIUserInterfaceStyle? { | ||
| switch styleString { | ||
| case "xlight", "light", | ||
| "systemUltraThinMaterialLight", "systemThinMaterialLight", | ||
| "systemMaterialLight", "systemThickMaterialLight", "systemChromeMaterialLight": | ||
| return .light | ||
| case "dark", "extraDark", | ||
| "systemUltraThinMaterialDark", "systemThinMaterialDark", | ||
| "systemMaterialDark", "systemThickMaterialDark", "systemChromeMaterialDark": | ||
| return .dark | ||
| default: | ||
| return nil | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Is it possible that at some point someone might request this adjustment to the blurring function through the system?
Perhaps we should consider doing this for both iOS and Android, adding a boolean property called adaptiveBlur. What do you think about this?
Summary by CodeRabbit
New Features
Bug Fixes
Chores