Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReworks iOS screen-capture protection into a policy-driven ScreenCaptureManager using AnimatedBlurEffectView; removes legacy runtime swizzles/extensions. Exposes Android blur methods with non-positive radius guard and scales radius by 10. Modernizes the example app (Swift, RN/Gradle/Pods/Metro/tooling), upgrades tooling/deps, and tweaks TypeScript/ignore settings. ChangesiOS Screen-capture & Blur Redesign
Android: Blur API Exposure & Behavior
Example App Modernization & Tooling
Repo config & TypeScript tweaks
Sequence Diagram(s)sequenceDiagram
actor User
participant RNApp as React Native App
participant Emm as Emm Module (bridge)
participant SCM as ScreenCaptureManager
participant Blur as AnimatedBlurEffectView
participant UI as Window/UI Layer
User->>RNApp: request blur (radius)
RNApp->>Emm: applyBlurEffect(radius)
Emm->>SCM: applyBlurEffect(intensity/animated)
alt intensity > 0
SCM->>Blur: create/configure AnimatedBlurEffectView(intensity)
Blur->>UI: insert overlay into last key window
Blur->>Blur: setupBlur() animate to fractionComplete
else intensity <= 0
SCM->>SCM: removeBlurEffect()
SCM->>UI: remove overlay
end
Note over SCM,RNApp: lifecycle events -> conditionalApply/conditionalRemove
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Microsoft Presidio Analyzer (2.2.362)ios/ScreenCaptureManager.swiftMicrosoft Presidio Analyzer failed to scan this file 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: 6
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/mattermost/emm/EmmModuleImpl.kt (1)
282-301:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the
10fmultiplier on blur radius.The
setBlurRadius(radius.toFloat() * 10f)call at line 301 rescales the public parameter unexpectedly. Sinceauthenticate()passes8.0, it becomes80f—far outside BlurView's typical range of 10–30f used in its examples and demo code. This changes the API contract silently; the radius parameter should be passed as-is to match library expectations and preserve the intended blur intensity.Proposed fix
- .setBlurRadius(radius.toFloat() * 10f) + .setBlurRadius(radius.toFloat())🤖 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/mattermost/emm/EmmModuleImpl.kt` around lines 282 - 301, The blur radius is being multiplied by 10 when configuring the BlurView which unexpectedly rescales the public parameter; in EmmModuleImpl.kt locate the BlurView setup block (where blurView is created and setupWith(rootView, blurAlgorithm) is called) and change the setBlurRadius call to pass the radius directly (use radius.toFloat()) instead of radius.toFloat() * 10f so the API contract and expected intensity from authenticate() are preserved.
🧹 Nitpick comments (2)
ios/AnimatedBlurEffectView.swift (1)
22-31: 💤 Low valueConsider stopping the animator on deinit.
The
UIViewPropertyAnimatorshould be explicitly stopped when the view is deallocated to ensure clean resource release. While the[weak self]capture prevents retain cycles, an active animator referencing a deallocated view could lead to unexpected behaviour.🛡️ Proposed fix to add cleanup
+ deinit { + animator.stopAnimation(true) + } + func setupBlur() {🤖 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/AnimatedBlurEffectView.swift` around lines 22 - 31, Add a deinit to AnimatedBlurEffectView that stops and finalizes the UIViewPropertyAnimator to avoid stray animations when the view is deallocated: in deinit call animator.stopAnimation(true), then animator.finishAnimation(at: .current) (or .end if you want to finalize to end state), and then clear any strong reference (e.g., set animator = nil) so the animator releases; update any related teardown logic if you have a different animator property name.example/package.json (1)
13-13: ⚡ Quick winMove
prettierto devDependencies.
prettieris a code formatting tool and should be a development dependency, not a runtime dependency.♻️ Proposed fix
"dependencies": { "@mattermost/react-native-emm": "../", - "prettier": "3.8.3", "react": "19.2.5", "react-native": "0.83.9" }, "devDependencies": { "@babel/core": "7.29.0", "@babel/runtime": "7.29.2", + "prettier": "3.8.3",🤖 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 13, The package.json currently lists "prettier": "3.8.3" as a runtime dependency; remove that entry from dependencies and add the same version under devDependencies (or run npm install --save-dev prettier@3.8.3) so Prettier is a development-only dependency; update package.json's dependencies/devDependencies accordingly and run the package manager to update the lockfile.
🤖 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/android/gradle/wrapper/gradle-wrapper.properties`:
- Line 3: The Gradle wrapper was bumped to 9.0.0 which is incompatible with
React Native 0.83.9's AGP 8.12.0; either revert the wrapper to a compatible 8.x
distribution (e.g., change distributionUrl in gradle-wrapper.properties back to
gradle-8.13-bin.zip) or instead update the Android Gradle Plugin to 9.0+ across
the project (adjust AGP version in the Gradle configuration / libs.versions.toml
and any RN-specific overrides) so AGP and the distributionUrl match; ensure
distributionUrl and AGP versions are consistent before merging.
In `@example/android/gradlew`:
- Line 117: The CLASSPATH variable in the POSIX gradle wrapper is assigned the
escaped-quote literal (CLASSPATH="\\\"\\\"") which propagates into the Java
args; change the assignment to an actual empty string (CLASSPATH="") or unset it
so it does not insert stray quotes into the Java command used when invoking the
wrapper (the CLASSPATH variable referenced in the wrapper invocation that builds
the Java args / -classpath or -jar gradle-wrapper.jar). Ensure any later
concatenation or use of CLASSPATH in the invocation logic treats an empty value
correctly (no extra quoting or literal backslashes).
In `@example/metro.config.js`:
- Around line 27-35: The current resolveRequest implementation may return a
non-existent base path when no extension matches; update the logic in
resolveRequest (the block handling moduleName === pak.name) to check whether the
computed filePath actually exists and, if not, either throw a clear error or log
a warning and delegate to the default resolver; specifically, after computing
base and extensions and attempting to find an existing file, if no file is found
(filePath === base or fs.existsSync(filePath) is false) throw a descriptive
Error that includes pak.name and base (or call processLogger.warn and return
context.resolveRequest(context, moduleName, platform)) so consumers don’t get an
invalid filePath.
In `@ios/Emm.swift`:
- Around line 17-23: The invalidate() method currently only removes observers
for self but captureEvents() registered observers on
ScreenCaptureManager.shared, causing leaked/duplicate observers; update
invalidate() to also remove the observers added on ScreenCaptureManager.shared
by calling
NotificationCenter.default.removeObserver(ScreenCaptureManager.shared) or
removing the specific notifications (UIApplication.willResignActiveNotification
and UIApplication.didBecomeActiveNotification) for ScreenCaptureManager.shared,
and keep the existing conditionalRemoveBlurEffect(forced: true) call; locate the
registration calls in captureEvents() and mirror their removal in invalidate().
In `@ios/ScreenCaptureManager.swift`:
- Around line 11-16: The didSet for preventScreenCapture currently only calls
setScreenCapturePolicy(), but must also re-register the capture listener so
screenCaptureStatusChange() receives UIScreen.capturedDidChangeNotification
updates; modify the preventScreenCapture.didSet to call listenForScreenCapture()
(or remove/rehook the notification observer via the same helper) whenever the
flag flips, ensuring listenForScreenCapture() subscribes/unsubscribes
appropriately and that setScreenCapturePolicy() continues to apply/remove layer
protection across view controllers.
- Around line 24-25: The selectors registered for
UIScreen.capturedDidChangeNotification and app lifecycle notifications expect
methods that accept a Notification, but conditionalApplyBlurEffect(intensity:)
and conditionalRemoveBlurEffect(forced:) have mismatched signatures; add `@objc`
wrapper methods like conditionalApplyBlurEffectNotification(_ notification:
Notification) and conditionalRemoveBlurEffectNotification(_ notification:
Notification) that extract any needed state (or call the public helpers with
default/derived arguments) and invoke conditionalApplyBlurEffect(intensity:) and
conditionalRemoveBlurEffect(forced:) respectively, then update the
NotificationCenter.addObserver calls (for
UIScreen.capturedDidChangeNotification,
UIApplication.willResignActiveNotification, and
UIApplication.didBecomeActiveNotification) to use these new wrapper selector
names so the selector signatures match NotificationCenter’s required (_
notification: Notification) form.
---
Outside diff comments:
In `@android/src/main/java/com/mattermost/emm/EmmModuleImpl.kt`:
- Around line 282-301: The blur radius is being multiplied by 10 when
configuring the BlurView which unexpectedly rescales the public parameter; in
EmmModuleImpl.kt locate the BlurView setup block (where blurView is created and
setupWith(rootView, blurAlgorithm) is called) and change the setBlurRadius call
to pass the radius directly (use radius.toFloat()) instead of radius.toFloat() *
10f so the API contract and expected intensity from authenticate() are
preserved.
---
Nitpick comments:
In `@example/package.json`:
- Line 13: The package.json currently lists "prettier": "3.8.3" as a runtime
dependency; remove that entry from dependencies and add the same version under
devDependencies (or run npm install --save-dev prettier@3.8.3) so Prettier is a
development-only dependency; update package.json's dependencies/devDependencies
accordingly and run the package manager to update the lockfile.
In `@ios/AnimatedBlurEffectView.swift`:
- Around line 22-31: Add a deinit to AnimatedBlurEffectView that stops and
finalizes the UIViewPropertyAnimator to avoid stray animations when the view is
deallocated: in deinit call animator.stopAnimation(true), then
animator.finishAnimation(at: .current) (or .end if you want to finalize to end
state), and then clear any strong reference (e.g., set animator = nil) so the
animator releases; update any related teardown logic if you have a different
animator property name.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: afdeb9b0-8268-4f35-8136-c8b4c2fc6b3c
⛔ Files ignored due to path filters (5)
example/Gemfile.lockis excluded by!**/*.lockexample/android/gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jarexample/ios/Podfile.lockis excluded by!**/*.lockexample/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
.gitignoreandroid/src/main/java/com/mattermost/emm/EmmModuleImpl.ktandroid/src/oldarch/java/com/EmmModule.ktexample/Gemfileexample/android/app/build.gradleexample/android/app/src/main/java/com/example/mattermostreactnativeemm/MainApplication.ktexample/android/build.gradleexample/android/gradle.propertiesexample/android/gradle/wrapper/gradle-wrapper.propertiesexample/android/gradlewexample/android/gradlew.batexample/babel.config.jsexample/ios/File.swiftexample/ios/Podfileexample/ios/ReactNativeEmmExample-Bridging-Header.hexample/ios/ReactNativeEmmExample.xcodeproj/project.pbxprojexample/ios/ReactNativeEmmExample/AppDelegate.hexample/ios/ReactNativeEmmExample/AppDelegate.mmexample/ios/ReactNativeEmmExample/AppDelegate.swiftexample/ios/ReactNativeEmmExample/Info.plistexample/ios/ReactNativeEmmExample/main.mexample/metro.config.jsexample/package.jsonios/AnimatedBlurEffectView.swiftios/Emm.mmios/Emm.swiftios/Extensions/UINavigationController+Swizzle.swiftios/Extensions/UIView+Shield.swiftios/Extensions/UIViewController+Shield.swiftios/Extensions/UIViewController+Swizzle.swiftios/Extensions/UIWindow+Swizzle.swiftios/ScreenCaptureManager.swiftpackage.jsonsrc/context.tsxtsconfig.json
💤 Files with no reviewable changes (11)
- example/ios/ReactNativeEmmExample/main.m
- example/ios/File.swift
- ios/Extensions/UIWindow+Swizzle.swift
- ios/Extensions/UIViewController+Shield.swift
- example/babel.config.js
- ios/Extensions/UIView+Shield.swift
- example/ios/ReactNativeEmmExample-Bridging-Header.h
- example/ios/ReactNativeEmmExample/AppDelegate.h
- ios/Extensions/UINavigationController+Swizzle.swift
- ios/Extensions/UIViewController+Swizzle.swift
- example/ios/ReactNativeEmmExample/AppDelegate.mm
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ios/Emm.swift (1)
15-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against duplicate observer registration on
ScreenCaptureManager.shared.If
captureEvents()is called multiple times (e.g., across JS bridge reloads), thewillResignActiveNotificationanddidBecomeActiveNotificationobservers will be registered multiple times on the singleton, causing duplicate blur callbacks. Consider adding a once-only guard.Proposed fix — add a static flag in ScreenCaptureManager
In
ScreenCaptureManager.swift, add a flag and registration method:private var hasRegisteredLifecycleObservers = false func registerLifecycleObserversOnce() { guard !hasRegisteredLifecycleObservers else { return } hasRegisteredLifecycleObservers = true NotificationCenter.default.addObserver( self, selector: `#selector`(handleWillResignActive(_:)), name: UIApplication.willResignActiveNotification, object: nil ) NotificationCenter.default.addObserver( self, selector: `#selector`(handleDidBecomeActive(_:)), name: UIApplication.didBecomeActiveNotification, object: nil ) }Then in
Emm.swift:`@objc` public func captureEvents() { NotificationCenter.default.addObserver(self, selector: `#selector`(managedConfigChaged(notification:)), name: UserDefaults.didChangeNotification, object: nil) - NotificationCenter.default.addObserver(ScreenCaptureManager.shared, selector: `#selector`(ScreenCaptureManager.handleWillResignActive(_:)), name: UIApplication.willResignActiveNotification, object: nil) - NotificationCenter.default.addObserver(ScreenCaptureManager.shared, selector: `#selector`(ScreenCaptureManager.handleDidBecomeActive(_:)), name: UIApplication.didBecomeActiveNotification, object: nil) + ScreenCaptureManager.shared.registerLifecycleObserversOnce() }Based on learnings: "guard against multiple registrations (e.g., check or use a once-once pattern) and document this lifecycle in code comments or tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/Emm.swift` around lines 15 - 19, captureEvents() currently registers ScreenCaptureManager.shared for willResignActiveNotification and didBecomeActiveNotification each time it's called, causing duplicate observers; fix by adding a once-only guard inside ScreenCaptureManager (e.g., a private hasRegisteredLifecycleObservers Bool and a public registerLifecycleObserversOnce() method) that registers those two notifications only if not already registered, then replace the direct NotificationCenter.addObserver calls for ScreenCaptureManager.shared in captureEvents() with a call to ScreenCaptureManager.shared.registerLifecycleObserversOnce(); ensure the flag and method live on ScreenCaptureManager and document the lifecycle behavior in a brief comment.
🧹 Nitpick comments (1)
ios/ScreenCaptureManager.swift (1)
98-108: 💤 Low valueConsider potential race on rapid
preventScreenCapturetoggles.The 0.25-second
asyncAfterdelay means rapid toggles ofpreventScreenCapturecould queue multiple async blocks. While the guards inapplyScreenCapturePolicyandremoveScreenCapturePolicyprovide some protection, the timing-dependent behavior could lead to unexpected states. Consider using a cancellation pattern or debounce mechanism if rapid toggling is a realistic scenario.🤖 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/ScreenCaptureManager.swift` around lines 98 - 108, The setScreenCapturePolicy function schedules a delayed block which can be queued multiple times during rapid toggles; change it to keep and cancel a stored DispatchWorkItem (or similar token) so each call cancels the previously scheduled work before creating a new one, then dispatch the new work item to call applyScreenCapturePolicy or removeScreenCapturePolicy on the main queue; update class state with a private var (e.g., pendingScreenCaptureWorkItem) and ensure you cancel and nil it when the work runs to implement a debounce/cancellation pattern for preventScreenCapture.
🤖 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 `@ios/ScreenCaptureManager.swift`:
- Around line 119-121: The handler handleCapturedDidChange currently only calls
conditionalApplyBlurEffect() and never removes the blur when capture stops;
update it to mirror screenCaptureStatusChange() behavior by checking
UIScreen.main.isCaptured and calling removeBlurEffect() when false (or simply
call screenCaptureStatusChange() from handleCapturedDidChange) so the blur is
removed on capture stop; refer to handleCapturedDidChange,
conditionalApplyBlurEffect(), screenCaptureStatusChange(), and
removeBlurEffect() to locate and implement the change.
- Around line 173-177: The non-animated fallback uses UIBlurEffect(style:
.regular) which mismatches the AnimatedBlurEffectView initialization using
.dark; update the else branch to apply the same blur style as the animated path
(use UIBlurEffect(style: .dark) or derive a single blurStyle constant used by
both AnimatedBlurEffectView init and blurEffectView.effect) so
AnimatedBlurEffectView and blurEffectView share the identical style and visual
appearance.
---
Duplicate comments:
In `@ios/Emm.swift`:
- Around line 15-19: captureEvents() currently registers
ScreenCaptureManager.shared for willResignActiveNotification and
didBecomeActiveNotification each time it's called, causing duplicate observers;
fix by adding a once-only guard inside ScreenCaptureManager (e.g., a private
hasRegisteredLifecycleObservers Bool and a public
registerLifecycleObserversOnce() method) that registers those two notifications
only if not already registered, then replace the direct
NotificationCenter.addObserver calls for ScreenCaptureManager.shared in
captureEvents() with a call to
ScreenCaptureManager.shared.registerLifecycleObserversOnce(); ensure the flag
and method live on ScreenCaptureManager and document the lifecycle behavior in a
brief comment.
---
Nitpick comments:
In `@ios/ScreenCaptureManager.swift`:
- Around line 98-108: The setScreenCapturePolicy function schedules a delayed
block which can be queued multiple times during rapid toggles; change it to keep
and cancel a stored DispatchWorkItem (or similar token) so each call cancels the
previously scheduled work before creating a new one, then dispatch the new work
item to call applyScreenCapturePolicy or removeScreenCapturePolicy on the main
queue; update class state with a private var (e.g.,
pendingScreenCaptureWorkItem) and ensure you cancel and nil it when the work
runs to implement a debounce/cancellation pattern for preventScreenCapture.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 225d0840-1b2c-46c2-9d61-2bce6bd109c2
📒 Files selected for processing (2)
ios/Emm.swiftios/ScreenCaptureManager.swift
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ios/ScreenCaptureManager.swift (1)
19-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrevent duplicate capture observer registrations.
When
preventScreenCaptureis set totruemore than once, this path can register the same observer repeatedly, causing duplicate callbacks.Proposed minimal fix
private func listenForScreenCapture() { if self.preventScreenCapture { + NotificationCenter.default.removeObserver( + self, + name: UIScreen.capturedDidChangeNotification, + object: nil + ) NotificationCenter.default.addObserver( self, selector: `#selector`(handleCapturedDidChange(_:)), name: UIScreen.capturedDidChangeNotification, object: nil ) return }🤖 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/ScreenCaptureManager.swift` around lines 19 - 27, The listenForScreenCapture method can add the same observer multiple times; before calling NotificationCenter.default.addObserver for UIScreen.capturedDidChangeNotification (in listenForScreenCapture) remove any existing observer registration for self and that notification (or use a stored observer token and check it) so you only register once; update listenForScreenCapture and the observer management around handleCapturedDidChange(_:) to either call NotificationCenter.default.removeObserver(self, name: UIScreen.capturedDidChangeNotification, object: nil) before adding or to store and reuse a single observer token to prevent duplicate callbacks.
🤖 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 `@ios/ScreenCaptureManager.swift`:
- Around line 115-117: The handler handleDidBecomeActive currently calls
conditionalRemoveBlurEffect(forced: true) which bypasses capture/auth checks and
can remove blur during an active screen capture; change this to not force
removal—call conditionalRemoveBlurEffect(forced: false) (or the
parameterless/explicit-safe overload) so the method honors ongoing capture/auth
state, ensuring conditionalRemoveBlurEffect still performs its capture/auth
checks before removing the blur.
---
Outside diff comments:
In `@ios/ScreenCaptureManager.swift`:
- Around line 19-27: The listenForScreenCapture method can add the same observer
multiple times; before calling NotificationCenter.default.addObserver for
UIScreen.capturedDidChangeNotification (in listenForScreenCapture) remove any
existing observer registration for self and that notification (or use a stored
observer token and check it) so you only register once; update
listenForScreenCapture and the observer management around
handleCapturedDidChange(_:) to either call
NotificationCenter.default.removeObserver(self, name:
UIScreen.capturedDidChangeNotification, object: nil) before adding or to store
and reuse a single observer token to prevent duplicate callbacks.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e700231-b8b1-4950-a43e-42a5aa18d8a4
📒 Files selected for processing (1)
ios/ScreenCaptureManager.swift
Contributor
Author
|
@coderabbitai resume |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ios/ScreenCaptureManager.swift`:
- Around line 174-178: The non-animated branch currently sets
blurEffectView.effect directly and ignores the stored intensity; instead, call
AnimatedBlurEffectView.setupBlur() and set the UIViewPropertyAnimator's
fractionComplete to the stored intensity so the configured blur intensity is
applied immediately—specifically update the else branch that now touches
blurEffectView to invoke setupBlur() on the AnimatedBlurEffectView instance and
set animator.fractionComplete = intensity (and ensure blurEffectView.alpha =
1.0) so the blur state is applied without animation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2b0eaac-a7f4-42d8-9504-41e8e0fbf8c3
📒 Files selected for processing (1)
ios/ScreenCaptureManager.swift
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ios/ScreenCaptureManager.swift`:
- Around line 8-9: applyScreenCapturePolicy() currently reparents the key window
but removeScreenCapturePolicy() calls getLastKeyWindow() again which can return
a different window; update the logic to capture and keep a strong reference to
the exact window/layer you protected when applying (e.g., store the key window
or its layer into a property like originalParentWindow/originalParent) and in
removeScreenCapturePolicy() restore that same stored layer back to its
originalParent instead of looking up getLastKeyWindow(); after restoring, clear
the stored references (protectionTextField and originalParent/
originalParentWindow) to avoid retention.
- Around line 38-43: Update the screen-capture condition so the blur remains
when a biometric auth flow has set isAuthenticating and blurOnAuthenticate even
if preventScreenCapture is false: in screenCaptureStatusChange() (and the
similar check at the other occurrence) replace the existing branch that requires
self.preventScreenCapture && UIScreen.main.isCaptured && !self.isAuthenticating
with a condition that applies the blur when UIScreen.main.isCaptured AND
(self.preventScreenCapture OR (self.isAuthenticating &&
self.blurOnAuthenticate)); remove the !self.isAuthenticating gating so
auth-triggered blur is preserved and otherwise call removeBlurEffect().
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40c56190-1681-446c-a4d9-27a38ee96e43
📒 Files selected for processing (1)
ios/ScreenCaptureManager.swift
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
ios/ScreenCaptureManager.swift (2)
8-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the same window that was originally protected.
applyScreenCapturePolicy()reparents whichever window is key at enable time, butremoveScreenCapturePolicy()asks for the key window again later. If focus changes while protection is active, removal can detach the wrong layer and leave the protected window stranded.Proposed fix
private var protectionTextField: UITextField? private var originalParent: CALayer? +private weak var protectedWindow: UIWindow? @@ private func applyScreenCapturePolicy() { guard let keyWindow = self.getLastKeyWindow(), + protectedWindow == nil, originalParent == nil, protectionTextField == nil else { return } @@ if let firstTextFieldSublayer = textField.layer.sublayers?.first { keyWindow.layer.removeFromSuperlayer() firstTextFieldSublayer.addSublayer(keyWindow.layer) } + protectedWindow = keyWindow protectionTextField = textField } @@ private func removeScreenCapturePolicy() { guard let textField = protectionTextField, - let window = self.getLastKeyWindow(), + let window = protectedWindow, let originalParentLayer = originalParent else { return } @@ textField.layer.removeFromSuperlayer() protectionTextField = nil originalParent = nil + protectedWindow = nil }Also applies to: 60-97
🤖 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/ScreenCaptureManager.swift` around lines 8 - 9, applyScreenCapturePolicy() currently reparents the current key window but removeScreenCapturePolicy() re-queries the key window, risking detaching the wrong layer if focus changed; fix by saving the exact original window/layer when you apply protection (use the existing originalParent and store a reference to the window or its layer at that time, e.g., originalParent and a new originalWindow or originalLayer variable) and in removeScreenCapturePolicy() always restore to that stored original reference (check nil and safely unparent/re-add protectionTextField to originalParent/originalLayer), clearing the stored references after restoration.
38-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the auth blur in place through
screenCaptureStatusChange().
ios/Emm.swift:30-40setsisAuthenticatingandblurOnAuthenticatebefore the biometric prompt, but this fallback branch still tears the overlay down wheneverpreventScreenCaptureisfalse. BecausehandleDidBecomeActive(_:)immediately routes through here, resuming the app mid-prompt can expose the protected UI.Proposed fix
func screenCaptureStatusChange() { if self.preventScreenCapture && UIScreen.main.isCaptured && !self.isAuthenticating { conditionalApplyBlurEffect() // Prevent screen recording - } else { + } else if !(self.isAuthenticating && self.blurOnAuthenticate) { removeBlurEffect() } }Also applies to: 116-118
🤖 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/ScreenCaptureManager.swift` around lines 38 - 43, Update screenCaptureStatusChange() so the blur is retained during authentication: change the if-condition that currently checks (preventScreenCapture && UIScreen.main.isCaptured && !self.isAuthenticating) to apply the blur when either screen-capture protection is triggered OR an authentication blur is requested. Concretely, call conditionalApplyBlurEffect() when ((preventScreenCapture && UIScreen.main.isCaptured) || (isAuthenticating && blurOnAuthenticate)) and only call removeBlurEffect() when neither case applies; also ensure any callers such as handleDidBecomeActive(_:) that route through screenCaptureStatusChange() will therefore preserve the auth blur during biometric prompts.
🤖 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 `@ios/ScreenCaptureManager.swift`:
- Around line 125-131: The fade-out completion currently unconditionally removes
blurEffectView, which drops any new blur requests made during the 250ms removal
because blurEffectView is non-nil; update the blur-removal completion to
re-check the current requirement ((preventScreenCapture && !isAuthenticating) ||
(isAuthenticating && blurOnAuthenticate)) before fully removing the view—if the
requirement now holds, restore or keep the blur (e.g., call
applyBlurEffect(intensity:animated:) or skip removal) instead of removing it;
apply the same re-check/update logic in the other blur-removal block referenced
around the second region (the block paired with conditionalApplyBlurEffect and
the corresponding hide/fade completion) so new requests during the animation are
honored.
- Around line 140-149: The Objective-C selector call in Emm.mm is using a
non-existent method signature; update the call site that references
applyBlurEffectWithIntensity:radius:animated: to call the exported Swift
selector applyBlurEffect:animated: on ScreenCaptureManager.shared instead, i.e.
replace the applyBlurEffectWithIntensity:... invocation with a call to
applyBlurEffect: animated: so it matches the `@objc` public func
applyBlurEffect(intensity:animated:) in ScreenCaptureManager.swift.
---
Duplicate comments:
In `@ios/ScreenCaptureManager.swift`:
- Around line 8-9: applyScreenCapturePolicy() currently reparents the current
key window but removeScreenCapturePolicy() re-queries the key window, risking
detaching the wrong layer if focus changed; fix by saving the exact original
window/layer when you apply protection (use the existing originalParent and
store a reference to the window or its layer at that time, e.g., originalParent
and a new originalWindow or originalLayer variable) and in
removeScreenCapturePolicy() always restore to that stored original reference
(check nil and safely unparent/re-add protectionTextField to
originalParent/originalLayer), clearing the stored references after restoration.
- Around line 38-43: Update screenCaptureStatusChange() so the blur is retained
during authentication: change the if-condition that currently checks
(preventScreenCapture && UIScreen.main.isCaptured && !self.isAuthenticating) to
apply the blur when either screen-capture protection is triggered OR an
authentication blur is requested. Concretely, call conditionalApplyBlurEffect()
when ((preventScreenCapture && UIScreen.main.isCaptured) || (isAuthenticating &&
blurOnAuthenticate)) and only call removeBlurEffect() when neither case applies;
also ensure any callers such as handleDidBecomeActive(_:) that route through
screenCaptureStatusChange() will therefore preserve the auth blur during
biometric prompts.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d605cebe-35f2-4292-a472-489ca2aefa7a
📒 Files selected for processing (1)
ios/ScreenCaptureManager.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ios/Emm.mm`:
- Around line 161-163: The Objective-C call inside Emm.mm's -applyBlurEffect: is
using the wrong selector; it calls [[ScreenCaptureManager shared]
applyBlurEffect:radius animated:true] but the Swift implementation exported as
applyBlurEffect(intensity:animated:) is available as
applyBlurEffect:intensity:animated:, so update the call to invoke the correct
selector or change the Swift method to export a simpler selector (add an
`@objc`("applyBlurEffect:animated:") on the Swift method); specifically, modify
the Emm.mm use of ScreenCaptureManager shared to call the selector matching
applyBlurEffect:intensity:animated: (passing radius as the intensity parameter)
or alternatively annotate the Swift method to `@objc`("applyBlurEffect:animated:")
so the existing ObjC call works.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ios/ScreenCaptureManager.swift`:
- Line 10: protectedWindow is declared weak so it may be nil out before
removeScreenCapturePolicy() runs, leaving protectionTextField and originalParent
latched; change protectedWindow from a weak var to a strong (non-weak) var to
retain the window while protection is active, assign the window into
protectedWindow when enabling protection (same place that currently sets
originalParent/protectionTextField), and ensure removeScreenCapturePolicy()
clears protectedWindow (set to nil) as part of its cleanup along with
protectionTextField and originalParent to avoid leaks and allow future
transitions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 94c80d36-b2b6-40ad-bd73-fcd3d374f6a6
📒 Files selected for processing (1)
ios/ScreenCaptureManager.swift
Contributor
Author
|
@coderabittai resolve |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AppDelegatefrom Objective-C (.mm/.h/main.m) to Swift, using the newRCTReactNativeFactory+RCTDefaultReactNativeFactoryDelegatepattern required by RN 0.83File.swift