fix: App hanging when changing passcode on iOS #6789
Conversation
|
|
WalkthroughWraps local authentication calls in try/catch in two views; on successful authentication waits MODAL_TRANSITION_DELAY_MS (300ms) before proceeding; on failure/cancel returns early. Adds MODAL_TRANSITION_DELAY_MS constant and new tests for timing, cancellation, and autoLock behavior. Exports ScreenLockConfigView publicly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant View as SecurityPrivacyView / ScreenLockConfigView
participant Auth as LocalAuthentication
participant Flow as Navigation / PasscodeFlow
User->>View: Request change/set passcode
View->>Auth: handleLocalAuthentication(true)
rect rgb(220,240,255)
Note over View,Auth: try/catch around auth call
Auth-->>View: Success
View->>View: wait MODAL_TRANSITION_DELAY_MS (300ms)
Note over View: Prevent iOS modal overlap before next modal
View->>Flow: navigate / changePasscode
end
rect rgb(255,235,235)
Auth-->>View: Failure / Cancel
Note over View: return early — abort navigation / passcode change
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (4)
🧰 Additional context used🧬 Code graph analysis (3)app/views/ScreenLockConfigView.test.tsx (1)
app/views/SecurityPrivacyView.tsx (2)
app/views/ScreenLockConfigView.tsx (2)
🔇 Additional comments (4)
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: 2
🧹 Nitpick comments (3)
app/views/ScreenLockConfigView.tsx (2)
135-143: Extract duplicated authentication pattern to a shared utility.The try-catch-delay pattern around
handleLocalAuthenticationis duplicated between this file (lines 135-143) and SecurityPrivacyView.tsx (lines 62-69). Consider extracting this to a shared utility function to reduce duplication and ensure consistent behavior.Example refactor in
app/lib/methods/helpers/localAuthentication.ts:export const handleLocalAuthenticationWithModalDelay = async (canCloseModal = false): Promise<boolean> => { try { await handleLocalAuthentication(canCloseModal); // Add a small delay to ensure the first modal is fully closed before opening the next one // This prevents the app from hanging on iOS when two modals open back-to-back await new Promise(resolve => setTimeout(resolve, 300)); return true; } catch { // User cancelled or authentication failed return false; } };Then simplify both callsites:
- try { - await handleLocalAuthentication(true); - // Add a small delay to ensure the first modal is fully closed before opening the next one - // This prevents the app from hanging on iOS when two modals open back-to-back - await new Promise(resolve => setTimeout(resolve, 300)); - } catch { - // User cancelled or authentication failed - return; - } + const authenticated = await handleLocalAuthenticationWithModalDelay(true); + if (!authenticated) { + return; + }
140-142: Consider logging authentication failures for debugging.The empty catch block silently handles all errors from
handleLocalAuthentication. While user cancellation is expected, other authentication errors might be valuable for debugging. Consider logging non-cancellation errors.} catch (error) { // User cancelled or authentication failed + if (error && error.code !== 'USER_CANCEL') { + console.warn('Authentication error:', error); + } return; }app/views/SecurityPrivacyView.tsx (1)
62-69: Apply the same refactoring as ScreenLockConfigView.tsx.This code duplicates the authentication-with-delay pattern from ScreenLockConfigView.tsx (lines 135-143). Once you extract the shared utility function suggested in that file, apply the same refactor here.
- try { - await handleLocalAuthentication(true); - // Add a small delay to prevent modal conflicts on iOS - await new Promise(resolve => setTimeout(resolve, 300)); - } catch { - // User cancelled or authentication failed - return; - } + const authenticated = await handleLocalAuthenticationWithModalDelay(true); + if (!authenticated) { + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/views/ScreenLockConfigView.tsx(1 hunks)app/views/SecurityPrivacyView.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/views/SecurityPrivacyView.tsx (1)
app/lib/methods/helpers/localAuthentication.ts (1)
handleLocalAuthentication(113-125)
app/views/ScreenLockConfigView.tsx (1)
app/lib/methods/helpers/localAuthentication.ts (1)
handleLocalAuthentication(113-125)
🔇 Additional comments (1)
app/views/ScreenLockConfigView.tsx (1)
139-139: Verify that 300ms delay is sufficient across iOS versions.The 300ms delay is crucial for preventing the iOS hang. Please verify that this duration is adequate across different iOS versions and device configurations (especially older devices with slower UI rendering).
Consider testing on:
- iOS 15, 16, 17, and 18
- Older devices (iPhone 8, iPhone X) alongside newer models
- Low-power mode scenarios
If 300ms proves insufficient in some cases, consider increasing to 500ms or making it configurable per-device.
DSingh0304
force-pushed
the
fix/passcode-change-hang-ios
branch
from
27bfc65 to
a86091f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/views/ScreenLockConfigView.test.tsx (2)
29-40: Extract duplicated simulation function.The
simulatePasscodeChangefunction is duplicated identically across three tests. This duplication should be eliminated.However, this becomes moot if you address the critical issue above and test the actual component instead of a simulation function.
Also applies to: 65-76, 94-104
48-49: Consider usingadvanceTimersByTime(300)for precision.Using
jest.runAllTimers()advances all pending timers regardless of their duration. For more precise verification of the 300ms delay, consider:-// Fast-forward timers to simulate the 300ms delay -jest.runAllTimers(); +// Fast-forward timers by exactly 300ms to verify the delay +jest.advanceTimersByTime(300);This makes it explicit that you're testing a 300ms delay specifically.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
app/views/ScreenLockConfigView.test.tsx(1 hunks)app/views/ScreenLockConfigView.tsx(1 hunks)app/views/SecurityPrivacyView.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/views/ScreenLockConfigView.tsx
- app/views/SecurityPrivacyView.tsx
DSingh0304
force-pushed
the
fix/passcode-change-hang-ios
branch
from
a86091f to
f23c341
Compare
- Add 300ms delay between authentication and passcode change modals - Add try-catch to handle authentication cancellation gracefully - Prevents iOS UI thread hang when two modals open back-to-back Fixes RocketChat#6313
DSingh0304
force-pushed
the
fix/passcode-change-hang-ios
branch
from
f23c341 to
4cb54a7
Compare
|
Hey maintainers, I would love it if someone could review the code and tell me if anything needs to be changed. |
2 participants
Proposed changes
This PR fixes an issue where the iOS app hangs when users attempt to change their passcode in the Screen Lock settings. The hang occurs due to two modals (authentication modal and passcode change modal) opening back-to-back, causing an iOS UI thread conflict.
The fix adds a 300ms delay between the authentication modal closing and the passcode change modal opening, allowing the first modal to fully dismiss before the second one appears. Additionally, try-catch blocks have been added to gracefully handle cases where users cancel authentication.
Issue(s)
Fixes #6313
How to test or reproduce
To reproduce the original bug (on iOS):
To verify the fix:
Testing:
yarn test app/views/ScreenLockConfigView.test.tsxFiles modified:
app/views/ScreenLockConfigView.tsx- Added delay and error handling tochangePasscodemethodapp/views/SecurityPrivacyView.tsx- Added delay and error handling tonavigateToScreenLockConfigViewmethodapp/views/ScreenLockConfigView.test.tsx- Added unit tests for the fixScreenshots
N/A - This is a timing/modal transition fix with no visual UI changes
Types of changes
Checklist
Further comments
This is a minimal, targeted fix that addresses the iOS-specific modal timing issue. The 300ms delay is a standard duration used in React Native for modal transitions and should be imperceptible to users while preventing the UI thread hang.
Unit tests have been added to verify:
Testing environment:
Summary by CodeRabbit
Bug Fixes
Chores
Tests