refactor: migrate ScreenLockConfigView to a function component#7434
refactor: migrate ScreenLockConfigView to a function component#7434diegolmello wants to merge 4 commits into
Conversation
Rewrite ScreenLockConfigView from a class component to a function component with hooks, part of the Migrate to Hooks effort. - Replace connect/withTheme with useAppSelector and useTheme; init runs in a mount effect; the title moves to a useLayoutEffect setOptions. - Convert the setState(updater, callback) toggles to compute the next value explicitly and run side effects (DB save, biometry read) on that value, avoiding stale-state reads after a setter. - Undo a cancelled passcode setup inline (set off + save off) instead of recursively re-toggling, firing the toggle analytics event once. - Remove the dead `observable` field (declared and unsubscribed but never assigned; no real subscription was dropped). - Register the screen via the bare static-config pattern in InsideStack and MasterDetailStack, matching DisplayPrefsView. - Add ScreenLockConfigView.test.tsx covering render, the stale-state persist guard, passcode-cancel undo, biometry toggle, and auto-lock time change. Claude-Session: https://claude.ai/code/session_01PA87xg21JfSiVH6cZbeceC
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
Walkthrough
ChangesScreen lock config rewrite
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
diegolmello
had a problem deploying
to
android_build
GitHub Actions
Error
|
|
||
| useLayoutEffect(() => { | ||
| navigation.setOptions({ title: I18n.t('Screen_lock') }); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
| setBiometryLabel(await supportedBiometryLabel()); | ||
| }; | ||
| init(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
![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
🧹 Nitpick comments (1)
app/stacks/InsideStack.tsx (1)
101-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid erasing the screen component type with
any.The direct registration is fine, but this cast hides static-screen prop mismatches. Prefer keeping the declared component contract without
any.Proposed cleanup
-const ScreenLockConfigViewScreen: ComponentType<StaticScreenProps<undefined>> = ScreenLockConfigView as any; +const ScreenLockConfigViewScreen: ComponentType<StaticScreenProps<undefined>> = ScreenLockConfigView;As per coding guidelines, TypeScript should be used for type safety.
🤖 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 `@app/stacks/InsideStack.tsx` at line 101, The ScreenLockConfigView screen registration is masking its props contract by casting to any, which defeats TypeScript safety. Update the ScreenLockConfigViewScreen declaration in InsideStack.tsx so it preserves the StaticScreenProps<undefined> component type without an any cast, and adjust ScreenLockConfigView if needed so its declared props match the expected screen contract.Source: Coding guidelines
🤖 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 `@app/views/ScreenLockConfigView.test.tsx`:
- Around line 108-122: The rollback test in ScreenLockConfigView only checks
that autoLock is reset, so it can miss stale timeout state after cancel. Update
the test around mockWrite/update in ScreenLockConfigView.test.tsx to also assert
the persisted record includes the default auto-lock duration from save(false,
DEFAULT_AUTO_LOCK), alongside the existing autoLock false check.
- Around line 16-22: The `useAppSelector` mock contracts in
`ScreenLockConfigView.test.tsx` are using `any`, which bypasses the
`IApplicationState` shape enforced by `useAppSelector`. Update the mocked
selector signatures in the test cases that stub `useAppSelector` to accept
`IApplicationState` instead of `any`, and keep the mocked state object typed
consistently with `server` and `settings` so drift is caught by TypeScript.
In `@app/views/ScreenLockConfigView.tsx`:
- Around line 75-82: The local screen-lock state in ScreenLockConfigView is
being initialized from the user server record even when Force_Screen_Lock is
enabled, which can leave autoLock false and hide the enforced duration UI.
Update the initialization in useEffect (and any dependent render logic for the
rows around the autoLock/autoLockTime controls) to derive state from the
effective forced screen-lock setting instead of raw serverRecord.current values,
so the switch and duration rows reflect the enforced configuration.
- Around line 103-143: The async UI handlers in ScreenLockConfigView should not
allow rejected promises to escape. Wrap toggleAutoLock and handleChangePasscode
in explicit try/catch so checkHasPasscode, handleLocalAuthentication,
changePasscode, and save failures are handled locally, and update the optimistic
state only after success or revert it in the catch path. In changeAutoLockTime,
do not fire save(autoLock, nextAutoLockTime) as a floating promise; await it or
attach explicit .catch handling so the save failure is surfaced and does not
become an unhandled rejection.
- Around line 147-163: The duration rows rendered by renderItem in
ScreenLockConfigView are returning an unkeyed fragment inside the items.map
loop, which can cause React reconciliation warnings. Update renderItem so the
top-level fragment or wrapper for the List.Item and List.Separator has a stable
key derived from the item identity, using the existing item fields in IItem and
keeping the key on the root element returned from the map.
- Around line 93-100: The save flow in ScreenLockConfigView should not silently
succeed when serverRecord.current is missing. In the save function, replace the
optional update on serverRecord.current with a guard that fails fast or
explicitly loads the record before entering database.servers.write, so the save
only completes after a valid record is available. Use save and
serverRecord.current as the key symbols when fixing this path.
---
Nitpick comments:
In `@app/stacks/InsideStack.tsx`:
- Line 101: The ScreenLockConfigView screen registration is masking its props
contract by casting to any, which defeats TypeScript safety. Update the
ScreenLockConfigViewScreen declaration in InsideStack.tsx so it preserves the
StaticScreenProps<undefined> component type without an any cast, and adjust
ScreenLockConfigView if needed so its declared props match the expected screen
contract.
🪄 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: bb7fcd55-5ad1-4d9b-ade7-2e9a60764330
📒 Files selected for processing (4)
app/stacks/InsideStack.tsxapp/stacks/MasterDetailStack/index.tsxapp/views/ScreenLockConfigView.test.tsxapp/views/ScreenLockConfigView.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/views/ScreenLockConfigView.test.tsxapp/stacks/InsideStack.tsxapp/stacks/MasterDetailStack/index.tsxapp/views/ScreenLockConfigView.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/views/ScreenLockConfigView.test.tsxapp/stacks/InsideStack.tsxapp/stacks/MasterDetailStack/index.tsxapp/views/ScreenLockConfigView.tsx
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/views/ScreenLockConfigView.test.tsxapp/stacks/InsideStack.tsxapp/stacks/MasterDetailStack/index.tsxapp/views/ScreenLockConfigView.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/views/ScreenLockConfigView.test.tsxapp/stacks/InsideStack.tsxapp/stacks/MasterDetailStack/index.tsxapp/views/ScreenLockConfigView.tsx
app/views/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place screen components in 'app/views/' directory
Files:
app/views/ScreenLockConfigView.test.tsxapp/views/ScreenLockConfigView.tsx
app/stacks/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place navigation stacks in 'app/stacks/' directory (InsideStack for authenticated, OutsideStack for login/register, MasterDetailStack for tablets, ShareExtensionStack)
Files:
app/stacks/InsideStack.tsxapp/stacks/MasterDetailStack/index.tsx
🧠 Learnings (2)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/views/ScreenLockConfigView.test.tsxapp/stacks/InsideStack.tsxapp/stacks/MasterDetailStack/index.tsxapp/views/ScreenLockConfigView.tsx
📚 Learning: 2026-06-24T22:58:43.390Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7157
File: app/views/MessagesView/index.tsx:392-392
Timestamp: 2026-06-24T22:58:43.390Z
Learning: When wrapping a React Native component (e.g., via `withSafeAreaInsets`) ensure `hoistNonReactStatics` is only required if the wrapped component actually defines static properties/methods that consumers rely on. If the component has no statics (as in `app/views/MessagesView/index.tsx`), you can omit `hoistNonReactStatics` for this case.
Applied to files:
app/views/ScreenLockConfigView.test.tsxapp/views/ScreenLockConfigView.tsx
🔇 Additional comments (4)
app/stacks/InsideStack.tsx (1)
240-240: LGTM!app/stacks/MasterDetailStack/index.tsx (1)
225-225: LGTM!app/views/ScreenLockConfigView.tsx (2)
1-6: LGTM!Also applies to: 19-22, 32-53
165-176: LGTM!Also applies to: 205-245, 249-249
Address review on ScreenLockConfigView: list the stable navigation and server dependencies on the title and init effects instead of suppressing the react-hooks/exhaustive-deps rule. Both values are referentially stable, so each effect still runs once on mount; no behavior change. Claude-Session: https://claude.ai/code/session_01PA87xg21JfSiVH6cZbeceC
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
had a problem deploying
to
android_build
GitHub Actions
Error
|
Hi cooked-pair, Thanks for submitting this pull request! Migrating Here are a few technical and constructive suggestions to further polish this migration: 1. Optimize Callback Stability with
|
- key the mapped auto-lock duration rows with a stable Fragment key - assert rolled-back autoLockTime in the passcode-cancel test - drop unnecessary `as any` from the screen registration Claude-Session: https://claude.ai/code/session_01PA87xg21JfSiVH6cZbeceC
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
had a problem deploying
to
android_build
GitHub Actions
Error
Shevilll
left a comment
Shevilll
left a comment
There was a problem hiding this comment.
Refactor Feedback: ScreenLockConfigView Migration to Function Component
Hi @diegolmello, thank you for this excellent migration to a functional component and React hooks! It's highly readable and properly solves the classic hooks state-persistence challenges.
I've reviewed the diff and have a couple of suggestions that can help prevent potential runtime bugs and optimize performance.
1. ⚠️ Potential Safety / Robustness Issue: Safe Destructuring in handleChangePasscode
In app/views/ScreenLockConfigView.tsx:
const handleChangePasscode = async ({ force }: { force: boolean }) => {
if (autoLock) {
await handleLocalAuthentication(true);
}
logEvent(events.SLC_CHANGE_PASSCODE);
await changePasscode({ force });
};And it is bound as an onPress callback:
<List.Item title='Local_authentication_change_passcode' onPress={handleChangePasscode} showActionIndicator />The Concern:
In React Native, onPress callback handlers typically receive a GestureResponderEvent as their first argument. While destructuring { force } from a GestureResponderEvent won't crash the application directly (since the event is an object, though force will evaluate to undefined or touch pressure on older iOS devices rather than a boolean), it can easily cause a crash in tests or custom setups if the onPress is called without arguments (e.g., onPress()).
If called with no arguments, the first argument is undefined, causing a runtime error:
TypeError: Cannot destructure property 'force' of 'undefined' as it is undefined.
Recommendation:
Provide a default parameter or handle the optional properties safely to ensure it is resilient under any callback trigger style:
const handleChangePasscode = async (args?: { force: boolean }) => {
const force = args?.force ?? false;
if (autoLock) {
await handleLocalAuthentication(true);
}
logEvent(events.SLC_CHANGE_PASSCODE);
await changePasscode({ force });
};2. ⚡ Performance Optimization: Memoizing Callbacks & Handlers
Since this component contains multiple interaction states and renders list sections, several internal handlers and elements are recreated on every render:
toggleAutoLocktoggleBiometryhandleChangePasscodechangeAutoLockTimerenderItem
The Concern:
Every render creates new references for these functions, which are passed down as props to child components like Switch, List.Item, etc. This will cause those child components to re-render unnecessarily on any state change (such as toggling auto-lock or biometrics).
Recommendation:
Consider wrapping key event handlers with useCallback to ensure stable references, and leverage useMemo for static items like items to keep the UI smooth:
const handleChangePasscode = useCallback(async (args?: { force: boolean }) => {
const force = args?.force ?? false;
if (autoLock) {
await handleLocalAuthentication(true);
}
logEvent(events.SLC_CHANGE_PASSCODE);
await changePasscode({ force });
}, [autoLock]);
const toggleAutoLock = useCallback(async () => {
logEvent(events.SLC_TOGGLE_AUTOLOCK);
const nextAutoLock = !autoLock;
setAutoLock(nextAutoLock);
setAutoLockTime(DEFAULT_AUTO_LOCK);
if (nextAutoLock) {
try {
await checkHasPasscode({ force: false });
setBiometry(userPreferences.getBool(BIOMETRY_ENABLED_KEY) ?? DEFAULT_BIOMETRY);
} catch {
setAutoLock(false);
setAutoLockTime(DEFAULT_AUTO_LOCK);
await save(false, DEFAULT_AUTO_LOCK);
return;
}
}
await save(nextAutoLock, DEFAULT_AUTO_LOCK);
}, [autoLock, save]);Thank you again for your hard work on cleaning up and migrating these views to functional components! Let me know what you think.
| if (this.observable && this.observable.unsubscribe) { | ||
| this.observable.unsubscribe(); | ||
| } | ||
| const defaultAutoLockOptions: IItem[] = [ |
There was a problem hiding this comment.
This is going to be instantiated on file import and it's going to use the language at that time. After a language change, the text is going to be outdated.
Btw can't we use i18n params?
I18n.t('Local_authentication_auto_lock', { value: 60 })
Move the auto-lock duration options out of module scope and build them inside renderAutoLockItems so I18n.t runs with the current language. At module scope the labels were translated once at import and went stale after a language change. Claude-Session: https://claude.ai/code/session_01PA87xg21JfSiVH6cZbeceC
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
diegolmello
temporarily deployed
to
android_build
GitHub Actions
Inactive
diegolmello
requested a deployment
to
upload_android
GitHub Actions
Waiting
|
Android Build Available Rocket.Chat 4.74.0.109211 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNSIJlzZGJFW8lbZwo1vif5slZLxCYuef3BXl9Lr9Zncm6GMtkL7GbiFTeuf27qfCDYiqj4aRkqVHad55nkC |
|
iOS Build Available Rocket.Chat 4.74.0.109212 |
2 participants
Proposed changes
Migrates the Screen Lock settings screen (
ScreenLockConfigView) — which controls the auto-lock timeout and the biometry (Face ID / Touch ID / fingerprint) toggle — from a class component to a function component with React hooks. Part of the ongoing Migrate to Hooks effort. Behavior is preserved.Notable points for review:
setState(updater, callback)and read the just-updated state inside the callback to decide whether to persist, prompt for a passcode, or undo. The function version computes the next value explicitly and persists that value, so there is no stale-state read after a state setter — the classic hooks pitfall this migration had to avoid.observablefield that was declared and unsubscribed but never assigned anywhere — no real subscription was dropped.InsideStackandMasterDetailStackwas updated to the current static-config navigation pattern (matching the already-migratedDisplayPrefsView), replacing the staticnavigationOptionswith auseLayoutEffectthat sets the title.Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-27
How to test or reproduce
Automated:
TZ=UTC pnpm test --testPathPattern='ScreenLockConfigView'— 5 tests: render after init, auto-lock enable persists, passcode-cancel undo, biometry toggle, and auto-lock-time change.Manual QA (Settings → Security and privacy → Screen lock):
Screenshots
No UI changes — this is a behavior-preserving refactor.
Types of changes
Checklist
Further comments
The core correctness property to preserve in any future change here: side effects (DB save, biometry read) must act on the computed next value, not on state read right after a
useStatesetter.https://claude.ai/code/session_01PA87xg21JfSiVH6cZbeceC
Summary by CodeRabbit
autoLockandautoLockTime, and safely reverts when enabling is canceled.