fix(suite-native): use react-native-keyboard-controller for keyboard handling #16838
Conversation
WalkthroughThe pull request replaces the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| keyboardShouldPersistTaps="handled" | ||
| contentInsetAdjustmentBehavior="automatic" | ||
| extraHeight={extraKeyboardAvoidingViewHeight} | ||
| // innerRef={ref => { |
There was a problem hiding this comment.
this should not be removed. useScrollView hook will not work without it. It is used on some screens where we want to scroll imperatively.
There was a problem hiding this comment.
Fixed in the latest version.
yanascz
force-pushed
the
fix/native/keyboard-aware-scroll-view
branch
from
2ca3a7b to
43ad751
Compare
|
🚀 Expo preview is ready!
|
yanascz
force-pushed
the
fix/native/keyboard-aware-scroll-view
branch
4 times, most recently
from
4122c2e to
e2772b4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-native/navigation/src/components/Screen.tsx (1)
20-32: Consider platform-specific keyboard behavior.The keyboard handling could be enhanced with platform-specific behavior.
Consider adding platform-specific behavior:
type ScreenProps = { children: ReactNode; header?: ReactNode; footer?: ReactNode; isScrollable?: boolean; backgroundColor?: Color; noHorizontalPadding?: boolean; noBottomPadding?: boolean; - extraKeyboardAvoidingViewHeight?: number; + keyboardBehavior?: 'height' | 'position' | 'padding'; hasBottomInset?: boolean; refreshControl?: ScrollViewProps['refreshControl']; keyboardDismissMode?: ScrollViewProps['keyboardDismissMode']; };Then update the KeyboardAvoidingView usage:
<KeyboardAvoidingView style={applyStyle(screenStyle)} keyboardVerticalOffset={spacings.sp16} - behavior="height" + behavior={keyboardBehavior ?? (Platform.OS === 'ios' ? 'padding' : 'height')} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
suite-native/app/package.json(1 hunks)suite-native/app/src/App.tsx(2 hunks)suite-native/module-accounts-import/src/components/AccountImportSummaryScreen.tsx(1 hunks)suite-native/module-accounts-import/src/screens/XpubScanScreen.tsx(0 hunks)suite-native/module-send/src/components/SendScreen.tsx(2 hunks)suite-native/navigation/package.json(1 hunks)suite-native/navigation/src/components/Screen.tsx(3 hunks)suite-native/navigation/src/components/ScreenContentWrapper.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- suite-native/module-accounts-import/src/screens/XpubScanScreen.tsx
⏰ Context from checks skipped due to timeout of 90000ms (22)
- GitHub Check: Type Checking
- GitHub Check: Releases revision Checks
- GitHub Check: Linting and formatting
- GitHub Check: Build libs for publishing
- GitHub Check: Other Checks
- GitHub Check: Unit Tests
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (7)
suite-native/module-send/src/components/SendScreen.tsx (1)
2-2: LGTM! Good implementation of keyboard handling.The changes properly integrate the new keyboard controller:
- KeyboardAvoidingView is correctly configured with height behavior
- Screen component is properly wrapped
- keyboardDismissMode="on-drag" improves UX by allowing keyboard dismissal on scroll
Also applies to: 13-17
suite-native/module-accounts-import/src/components/AccountImportSummaryScreen.tsx (1)
26-30: LGTM! Simplified footer implementation.Removing the redundant KeyboardAvoidingView is correct since keyboard handling is now managed by the KeyboardProvider at the app level.
suite-native/navigation/src/components/ScreenContentWrapper.tsx (1)
34-47: LGTM! Proper migration to standard ScrollView.The changes correctly:
- Maintain ScrollViewContext functionality
- Remove redundant keyboard handling as it's now managed by KeyboardProvider
- Preserve essential ScrollView props for keyboard interaction
suite-native/app/src/App.tsx (1)
3-3: LGTM! Proper integration of KeyboardProvider.The KeyboardProvider is correctly placed high in the component tree to manage keyboard behavior app-wide.
Let's verify the react-native-keyboard-controller version and compatibility:
Also applies to: 84-92
✅ Verification successful
🌐 Web query:
What is the latest version of react-native-keyboard-controller and its compatibility with React Native?💡 Result:
The latest version of **react-native-keyboard-controller** is **1.16.2** (released a day ago as of February 2025)[1][4]. This library maintains compatibility across modern React Native versions and platforms through continuous updates: ### Key Compatibility Features | Aspect | Details | |--------|---------| | **React Native Support** | Works with any navigation library and tested up to RN 0.75[1][4], though no incompatibilities reported with newer versions like RN 0.77[6] | | **Platform Parity** | Identical iOS/Android behavior for keyboard events and animations[2][4] | | **Architecture Support** | Compatible with both old and new React Native architectures (Fabric)[8] | | **Third-Party Integration** | Works with `@gorhom/bottom-sheet`[5], `react-native-gesture-handler`[1], and `react-native-reanimated`[4] | Recent updates include critical fixes for Stripe inputs[1], XCode warnings[1], and Android crashes[1][3]. The library supports modern Android/iOS APIs while maintaining backward compatibility (e.g., iOS <13 support via polyfills)[1][3]. For installation: ```bash yarn add [email protected] # or npm install [email protected] ``` The documentation confirms compatibility with "any React Native version"[4], though testing with specific RN releases (especially beyond 0.75) is recommended during upgrades[6]. Citations: - 1: https://github.com/kirillzyusko/react-native-keyboard-controller/releases - 2: https://www.npmjs.com/package/react-native-keyboard-controller/v/1.0.0-alpha.1 - 3: https://kirillzyusko.github.io/react-native-keyboard-controller/blog/release-1-13 - 4: https://www.npmjs.com/package/react-native-keyboard-controller - 5: https://github.com/kirillzyusko/react-native-keyboard-controller/discussions/309 - 6: https://reactnative.dev/blog/2025/01/21/version-0.77 - 7: https://kirillzyusko.github.io/react-native-keyboard-controller/blog - 8: https://github.com/kirillzyusko/react-native-keyboard-controller/blob/main/react-native-keyboard-controller.podspec - 9: https://reactnative.dev/versions - 10: https://kirillzyusko.github.io/react-native-keyboard-controller/
LGTM: KeyboardProvider integration is correctly implemented.
- Confirmed using the web query that the latest version of react-native-keyboard-controller is 1.16.2.
- This version supports modern React Native versions (tested up to RN 0.75 and known to work with newer versions like RN 0.77), ensuring compatibility and proper keyboard handling.
suite-native/navigation/src/components/Screen.tsx (1)
123-127: Verify keyboard behavior across different screen layouts.The implementation looks good, but the fixed
behavior="height"setting might not be optimal for all screens, especially those with complex layouts.Consider testing the following scenarios:
- Forms at the bottom of scrollable content
- Text inputs in the middle of the screen
- Landscape orientation handling
suite-native/navigation/package.json (1)
36-36: Verify the stability of react-native-keyboard-controller.Let's check if version 1.16.2 is the latest stable version and review its compatibility with React Native 0.76.1.
✅ Verification successful
🌐 Web query:
What is the latest stable version of react-native-keyboard-controller and its compatibility with React Native 0.76.1?💡 Result:
The latest stable version of **react-native-keyboard-controller** is **1.16.2** (published a day ago as of February 2025) [3][5]. This version maintains full compatibility with **React Native 0.76.x**, including **0.76.1** [2][4]. ### Key Compatibility Details: - **Fabric Architecture Support**: Version **1.13.0+** explicitly supports React Native **0.75+**, including subsequent updates like **0.76.x** [2]. - **Tested Features**: 1.x versions have been validated across multiple iOS/Android versions and RN architectures (Paper/Fabric) [1][5]. - **RN 0.76-Specific Enhancements**: Recent updates address edge cases in keyboard handling introduced by React Native's New Architecture default in v0.76 [4][5]. For projects using React Native ≥0.76, the recommended minimum keyboard-controller version is **1.13.x**, though upgrading to **1.16.x** is advised for bug fixes and new features like async dismiss handling and performance optimizations [5]. Citations: - 1: https://kirillzyusko.github.io/react-native-keyboard-controller/blog/release-1-13 - 2: https://kirillzyusko.github.io/react-native-keyboard-controller/docs/guides/compatibility - 3: https://www.npmjs.com/package/react-native-keyboard-controller - 4: https://reactnative.dev/blog/2024/10/23/release-0.76-new-architecture - 5: https://github.com/kirillzyusko/react-native-keyboard-controller/releases - 6: https://reactnative.dev/versions - 7: https://kirillzyusko.github.io/react-native-keyboard-controller/ - 8: https://github.com/facebook/react-native/blob/main/CHANGELOG.md?plain=1 - 9: https://github.com/kirillzyusko/react-native-keyboard-controller/issues/327 - 10: https://expo.dev/changelog/2024/10-24-sdk-52-beta
Stable Version and Compatibility Confirmed
The latest stable version of react-native-keyboard-controller is indeed 1.16.2—recently published—and it fully supports React Native 0.76.1 (as part of the 0.76.x compatibility range). No changes are required.suite-native/app/package.json (1)
115-115: LGTM! Consistent dependency versioning.The version of react-native-keyboard-controller matches the one in the navigation package, maintaining consistency across the codebase.
yanascz
force-pushed
the
fix/native/keyboard-aware-scroll-view
branch
2 times, most recently
from
494256e to
9407245
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
suite-native/navigation/src/components/Screen.tsx (1)
20-32: Remove unused prop type.The
extraKeyboardAvoidingViewHeightprop is defined but no longer used after switching toreact-native-keyboard-controller.Apply this diff to remove the unused prop:
type ScreenProps = { children: ReactNode; header?: ReactNode; footer?: ReactNode; isScrollable?: boolean; backgroundColor?: Color; noHorizontalPadding?: boolean; noBottomPadding?: boolean; - extraKeyboardAvoidingViewHeight?: number; hasBottomInset?: boolean; refreshControl?: ScrollViewProps['refreshControl']; keyboardDismissMode?: ScrollViewProps['keyboardDismissMode']; };
🧹 Nitpick comments (1)
suite-native/navigation/src/components/Screen.tsx (1)
123-127: Consider platform-specific keyboard avoiding behavior.While "height" behavior works, iOS typically uses "padding" behavior for smoother keyboard interactions. Consider using platform-specific behavior:
<KeyboardAvoidingView style={applyStyle(screenStyle)} keyboardVerticalOffset={spacings.sp16} - behavior="height" + behavior={Platform.OS === 'ios' ? 'padding' : 'height'} >Don't forget to add the import:
+import { Platform } from 'react-native';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
suite-native/app/package.json(1 hunks)suite-native/app/src/App.tsx(2 hunks)suite-native/module-accounts-import/src/components/AccountImportSummaryScreen.tsx(1 hunks)suite-native/module-accounts-import/src/screens/XpubScanScreen.tsx(0 hunks)suite-native/navigation/package.json(1 hunks)suite-native/navigation/src/components/Screen.tsx(3 hunks)suite-native/navigation/src/components/ScreenContentWrapper.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- suite-native/module-accounts-import/src/screens/XpubScanScreen.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- suite-native/app/package.json
- suite-native/navigation/package.json
- suite-native/app/src/App.tsx
- suite-native/module-accounts-import/src/components/AccountImportSummaryScreen.tsx
- suite-native/navigation/src/components/ScreenContentWrapper.tsx
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: prepare_android_test_app
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (3)
suite-native/navigation/src/components/Screen.tsx (3)
4-4: LGTM! Import of KeyboardAvoidingView.The import aligns with the PR objective to use
react-native-keyboard-controllerfor keyboard handling.
34-36: LGTM! Simple flex style for KeyboardAvoidingView.The style ensures the keyboard avoiding view takes full available space.
128-159: LGTM! Clean implementation of screen layout.The screen layout maintains proper structure with SystemBars, header, content, and footer while supporting keyboard avoidance.
yanascz
force-pushed
the
fix/native/keyboard-aware-scroll-view
branch
from
9407245 to
bcefc03
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-native/navigation/src/components/Screen.tsx (1)
123-127: Consider making keyboard avoidance behavior configurable.While the current implementation works, different screens might require different keyboard avoidance behaviors. Consider exposing these props:
keyboardVerticalOffsetfor custom offset valuesbehaviorto support different adjustment strategies ("height", "position", "padding")type ScreenProps = { children: ReactNode; header?: ReactNode; footer?: ReactNode; + keyboardVerticalOffset?: number; + keyboardAvoidingBehavior?: 'height' | 'position' | 'padding'; // ... other props }; export const Screen = ({ children, header, footer, + keyboardVerticalOffset = spacings.sp16, + keyboardAvoidingBehavior = 'height', // ... other props }: ScreenProps) => { // ... other code return ( <KeyboardAvoidingView style={applyStyle(screenStyle)} - keyboardVerticalOffset={spacings.sp16} - behavior="height" + keyboardVerticalOffset={keyboardVerticalOffset} + behavior={keyboardAvoidingBehavior} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
scripts/list-outdated-dependencies/mobile-dependencies.txt(1 hunks)suite-native/app/package.json(1 hunks)suite-native/app/src/App.tsx(2 hunks)suite-native/module-accounts-import/src/components/AccountImportSummaryScreen.tsx(1 hunks)suite-native/module-accounts-import/src/screens/XpubScanScreen.tsx(0 hunks)suite-native/navigation/package.json(1 hunks)suite-native/navigation/src/components/Screen.tsx(3 hunks)suite-native/navigation/src/components/ScreenContentWrapper.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- suite-native/module-accounts-import/src/screens/XpubScanScreen.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- suite-native/navigation/package.json
- suite-native/module-accounts-import/src/components/AccountImportSummaryScreen.tsx
- suite-native/navigation/src/components/ScreenContentWrapper.tsx
- suite-native/app/src/App.tsx
- scripts/list-outdated-dependencies/mobile-dependencies.txt
- suite-native/app/package.json
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: transport-e2e-test
🔇 Additional comments (3)
suite-native/navigation/src/components/Screen.tsx (3)
4-4: LGTM! Import and type changes align with the new keyboard handling approach.The changes correctly integrate the new keyboard controller library while cleaning up the obsolete keyboard height prop.
Also applies to: 20-32
34-36: LGTM! Style definition ensures proper keyboard avoidance behavior.The
flex: 1style is essential forKeyboardAvoidingViewto calculate and adjust its dimensions correctly.
128-159: LGTM! Component hierarchy maintains proper layout structure.The nested components are correctly organized within the
KeyboardAvoidingView, ensuring proper keyboard handling while preserving the existing layout behavior.
yanascz
force-pushed
the
fix/native/keyboard-aware-scroll-view
branch
2 times, most recently
from
de09d27 to
f3e8a3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
suite-native/navigation/src/components/Screen.tsx (1)
28-28: Remove unused prop type.The
extraKeyboardAvoidingViewHeightprop is no longer used in the component implementation since keyboard handling is now managed byreact-native-keyboard-controller.Apply this diff to remove the unused prop:
- extraKeyboardAvoidingViewHeight?: number;
🧹 Nitpick comments (1)
suite-native/navigation/src/components/Screen.tsx (1)
123-127: Consider platform-specific keyboard behavior.While
behavior="height"works well on Android, iOS typically benefits from using"padding"behavior for smoother keyboard interactions.Consider updating the behavior prop to be platform-specific:
- behavior="height" + behavior={Platform.OS === 'ios' ? 'padding' : 'height'}Don't forget to add the Platform import:
-import { ScrollViewProps, View } from 'react-native'; +import { Platform, ScrollViewProps, View } from 'react-native';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
scripts/list-outdated-dependencies/mobile-dependencies.txt(1 hunks)suite-native/app/package.json(1 hunks)suite-native/app/src/App.tsx(2 hunks)suite-native/module-accounts-import/src/components/AccountImportSummaryScreen.tsx(1 hunks)suite-native/module-accounts-import/src/screens/XpubScanScreen.tsx(0 hunks)suite-native/navigation/package.json(1 hunks)suite-native/navigation/src/components/Screen.tsx(3 hunks)suite-native/navigation/src/components/ScreenContentWrapper.tsx(2 hunks)suite-native/test-utils/package.json(1 hunks)suite-native/test-utils/src/expoMock.js(1 hunks)
💤 Files with no reviewable changes (1)
- suite-native/module-accounts-import/src/screens/XpubScanScreen.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- suite-native/module-accounts-import/src/components/AccountImportSummaryScreen.tsx
- suite-native/test-utils/src/expoMock.js
- suite-native/test-utils/package.json
- suite-native/navigation/src/components/ScreenContentWrapper.tsx
- suite-native/navigation/package.json
- suite-native/app/package.json
- scripts/list-outdated-dependencies/mobile-dependencies.txt
- suite-native/app/src/App.tsx
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: transport-e2e-test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: test
🔇 Additional comments (2)
suite-native/navigation/src/components/Screen.tsx (2)
34-36: LGTM!The new
screenStylecorrectly ensures theKeyboardAvoidingViewtakes up the full screen space.
95-106: Add documentation for keyboard handling behavior.Since this component implements critical keyboard handling logic, it would be beneficial to:
- Document the keyboard behavior in the component's JSDoc.
- Test the keyboard interactions on both iOS and Android devices.
Could you verify the keyboard behavior works as expected on both platforms? Pay special attention to:
- Keyboard appearance/disappearance animations
- Content shifting when keyboard shows/hides
- Interaction with different input fields
yanascz
force-pushed
the
fix/native/keyboard-aware-scroll-view
branch
from
f3e8a3a to
7996c6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
suite-native/navigation/src/components/Screen.tsx (1)
123-127: Consider platform-specific keyboard behavior.While the "height" behavior works well on iOS, Android might benefit from a different behavior. Consider using platform-specific behavior:
<KeyboardAvoidingView style={applyStyle(screenStyle)} keyboardVerticalOffset={spacings.sp16} - behavior="height" + behavior={Platform.OS === 'ios' ? 'height' : 'padding'} >Don't forget to add the import:
+import { Platform } from 'react-native';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
scripts/list-outdated-dependencies/mobile-dependencies.txt(1 hunks)suite-native/app/package.json(1 hunks)suite-native/app/src/App.tsx(2 hunks)suite-native/module-accounts-import/src/components/AccountImportSummaryScreen.tsx(1 hunks)suite-native/module-accounts-import/src/screens/XpubScanScreen.tsx(0 hunks)suite-native/navigation/package.json(1 hunks)suite-native/navigation/src/components/Screen.tsx(3 hunks)suite-native/navigation/src/components/ScreenContentWrapper.tsx(2 hunks)suite-native/test-utils/package.json(1 hunks)suite-native/test-utils/src/expoMock.js(1 hunks)
💤 Files with no reviewable changes (1)
- suite-native/module-accounts-import/src/screens/XpubScanScreen.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- suite-native/test-utils/package.json
- scripts/list-outdated-dependencies/mobile-dependencies.txt
- suite-native/module-accounts-import/src/components/AccountImportSummaryScreen.tsx
- suite-native/navigation/package.json
- suite-native/app/src/App.tsx
- suite-native/navigation/src/components/ScreenContentWrapper.tsx
- suite-native/app/package.json
- suite-native/test-utils/src/expoMock.js
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: prepare_android_test_app
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (2)
suite-native/navigation/src/components/Screen.tsx (2)
34-36: LGTM!The new
screenStylecorrectly ensures theKeyboardAvoidingViewtakes full available space.
125-125: Verify keyboard vertical offset.The
sp16spacing might not be sufficient for all screen sizes and orientations. Consider making this value dynamic based on the device's screen size or navigation bar height.
yanascz
force-pushed
the
fix/native/keyboard-aware-scroll-view
branch
3 times, most recently
from
ccbdcaa to
5004fa1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
suite-native/module-send/src/screens/SendAccountsScreen.tsx (1)
11-13: Track these TODOs in separate issues.There are three TODOs that should be tracked:
- Remove Bitcoin-like coins filter
- Move text content to @suite-native/intl package
- Enable filtering similar to receive screen
Would you like me to create separate issues to track these TODOs?
Also applies to: 25-25, 28-28
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (21)
scripts/list-outdated-dependencies/mobile-dependencies.txt(1 hunks)suite-native/app/package.json(1 hunks)suite-native/app/src/App.tsx(2 hunks)suite-native/module-accounts-import/src/components/AccountImportSummaryScreen.tsx(3 hunks)suite-native/module-accounts-import/src/screens/XpubScanScreen.tsx(3 hunks)suite-native/module-authorize-device/src/components/passphrase/PassphraseContentScreenWrapper.tsx(2 hunks)suite-native/module-authorize-device/src/screens/passphrase/PassphraseEnterOnTrezorScreen.tsx(1 hunks)suite-native/module-authorize-device/src/screens/passphrase/PassphraseFormScreen.tsx(3 hunks)suite-native/module-authorize-device/src/screens/passphrase/PassphraseVerifyEmptyWalletScreen.tsx(3 hunks)suite-native/module-send/src/components/SendScreen.tsx(0 hunks)suite-native/module-send/src/screens/SendAccountsScreen.tsx(2 hunks)suite-native/module-send/src/screens/SendAddressReviewScreen.tsx(3 hunks)suite-native/module-send/src/screens/SendDestinationTagReviewScreen.tsx(3 hunks)suite-native/module-send/src/screens/SendFeesScreen.tsx(2 hunks)suite-native/module-send/src/screens/SendOutputsReviewScreen.tsx(3 hunks)suite-native/module-send/src/screens/SendOutputsScreen.tsx(3 hunks)suite-native/navigation/package.json(1 hunks)suite-native/navigation/src/components/Screen.tsx(2 hunks)suite-native/navigation/src/components/ScreenContentWrapper.tsx(2 hunks)suite-native/test-utils/package.json(1 hunks)suite-native/test-utils/src/expoMock.js(1 hunks)
💤 Files with no reviewable changes (1)
- suite-native/module-send/src/components/SendScreen.tsx
✅ Files skipped from review due to trivial changes (1)
- suite-native/module-authorize-device/src/screens/passphrase/PassphraseEnterOnTrezorScreen.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- suite-native/test-utils/package.json
- suite-native/module-accounts-import/src/components/AccountImportSummaryScreen.tsx
- scripts/list-outdated-dependencies/mobile-dependencies.txt
- suite-native/navigation/src/components/ScreenContentWrapper.tsx
- suite-native/app/src/App.tsx
- suite-native/test-utils/src/expoMock.js
- suite-native/app/package.json
- suite-native/navigation/package.json
- suite-native/module-accounts-import/src/screens/XpubScanScreen.tsx
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build
- GitHub Check: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (21)
suite-native/module-send/src/screens/SendFeesScreen.tsx (2)
4-4: LGTM! Import changes align with the PR objectives.The consolidation of imports and switch to
Screenfrom@suite-native/navigationis consistent with the keyboard handling improvements being implemented across the codebase.
20-26: LGTM! Component changes maintain functionality while improving keyboard handling.The replacement of
SendScreenwithScreenand the prop name update fromscreenHeadertoheaderare consistent with the changes being made across the codebase to improve keyboard handling.suite-native/module-send/src/screens/SendOutputsScreen.tsx (3)
29-29: LGTM!The import of
KeyboardAvoidingScreenaligns with the PR's objective of improving keyboard handling.
266-268: LGTM!The replacement of
SendScreenwithKeyboardAvoidingScreenimproves keyboard handling as intended.
289-293: Verify margin's impact on keyboard avoidance.The vertical margin around the form content might affect the keyboard avoidance behavior. Please test thoroughly to ensure the form fields remain accessible when the keyboard is visible.
suite-native/module-authorize-device/src/components/passphrase/PassphraseContentScreenWrapper.tsx (2)
3-3: LGTM! Clean and consistent implementation.The refactoring improves code maintainability by using the
TitleHeadercomponent consistently across screens.Also applies to: 21-21
3-3: LGTM! Clean refactoring of the header implementation.The replacement of nested Text components with a single TitleHeader component improves code maintainability while preserving the component's functionality.
Also applies to: 21-21
suite-native/module-authorize-device/src/screens/passphrase/PassphraseVerifyEmptyWalletScreen.tsx (2)
7-8: LGTM! Consistent keyboard handling implementation.The migration to
KeyboardAvoidingScreenaligns with the PR's objective of usingreact-native-keyboard-controllerfor improved keyboard handling.Also applies to: 13-13, 38-48
7-8: LGTM! Improved keyboard handling implementation.The migration to KeyboardAvoidingScreen enhances the keyboard interaction while maintaining the screen's functionality.
Also applies to: 13-13, 38-48
suite-native/module-authorize-device/src/screens/passphrase/PassphraseFormScreen.tsx (2)
74-87: LGTM! Clean migration to KeyboardAvoidingScreen.The changes successfully integrate the new keyboard handling while preserving the existing animation logic.
Also applies to: 164-164
5-9: LGTM! Successful migration to improved keyboard handling.The changes effectively integrate the KeyboardAvoidingScreen while preserving the existing animation and interaction logic.
Also applies to: 74-87, 164-164
suite-native/module-send/src/screens/SendAddressReviewScreen.tsx (4)
8-8: LGTM!The import of
Screenfrom@suite-native/navigationis correctly added to the existing import statement.
46-48: LGTM!The transition from
SendScreentoScreencomponent is correctly implemented with the appropriate prop name update fromscreenHeadertoheader.
64-64: LGTM!The closing tag correctly matches the new
Screencomponent.
53-53: Verify the status of the illustration improvement.There's a TODO comment referencing issue #13965 for improving the illustration.
Let's check the status of this issue:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check the status of the illustration improvement issue gh issue view 13965 --json title,state,bodyLength of output: 453
Action Required: Verify implementation of the improved illustration in the send flow.
The referenced GitHub issue (#13965) is now CLOSED, which indicates that the work on reusing the
ConfirmOnTrezorImagecomponent for the send flow has been concluded. However, the fact that the TODO comment remains in the code suggests that there might be pending cleanup/documentation work, or that the improvement might not be fully integrated yet.Please verify if:
- The
ConfirmOnTrezorImagecomponent is correctly integrated into the send flow (i.e., clicking the illustration triggers the expected bottom sheet as per the acceptance criteria).- Any remaining TODO/comment has been scheduled for removal or update once the feature is fully completed.
suite-native/module-send/src/screens/SendAccountsScreen.tsx (1)
4-4: LGTM! Verify component replacement across the codebase.The replacement of
SendScreenwithScreenand the prop name update look good. This change aligns with the PR's objective to improve keyboard handling.Let's verify that this component replacement pattern is consistent across other files:
Also applies to: 27-28, 34-34
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining usage of SendScreen component # and verify Screen component usage pattern # Check for any remaining SendScreen imports or usage rg -l "SendScreen" "suite-native/module-send/src" # Check Screen component usage pattern in other files rg -A 2 "<Screen\s+header={" "suite-native/module-send/src"Length of output: 207
I'll run a new shell script using a corrected regex (escaping the curly brace) to verify the usage of both the removed
SendScreenand the newScreencomponent.
🏁 Script executed:
#!/bin/bash # Verify that there's no usage of the removed SendScreen component rg -l "SendScreen" "suite-native/module-send/src" # Verify that the new Screen component is used with the expected prop "header" # Using an escaped { in the regex rg -A 2 "<Screen\s+header=\{" "suite-native/module-send/src"Length of output: 494
Component Replacement Verified
The search confirms that there are no remaining references to
SendScreenin the codebase and that theScreencomponent is correctly used with the updatedheaderprop in theSendAccountsScreen.tsxfile.suite-native/module-send/src/screens/SendDestinationTagReviewScreen.tsx (1)
10-10: LGTM! Component structure changes align with standardization efforts.The replacement of
SendScreenwithScreenand the prop name change fromscreenHeadertoheaderimprove consistency across the application.Let's verify that the
Screencomponent includes the new keyboard handling capabilities:Also applies to: 68-74, 83-83
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check if Screen component uses KeyboardAvoidingView from react-native-keyboard-controller ast-grep --pattern $'import { KeyboardAvoidingView } from "react-native-keyboard-controller" export const Screen = ({ $_ }) => { return ( <KeyboardAvoidingView $$$ > $$$ </KeyboardAvoidingView> ) }'Length of output: 228
Keyboard Handling Verification – Manual Confirmation Required
The refactor in the SendDestinationTagReviewScreen is correctly applying the Screen component with updated prop names; however, our automated search did not uncover evidence that the Screen component explicitly incorporates the new keyboard handling (e.g., via a direct use of KeyboardAvoidingView from react-native-keyboard-controller).
Please verify manually that the Screen component correctly integrates the new keyboard handling logic—whether by an explicit KeyboardAvoidingView wrapper or through an alternative internal mechanism—to ensure the PR’s objectives for enhanced keyboard management are met.
suite-native/navigation/src/components/Screen.tsx (2)
28-28: Remove unused prop type.The
extraKeyboardAvoidingViewHeightprop is defined but no longer used after migrating toreact-native-keyboard-controller.
153-175: LGTM! Well-implemented keyboard handling.The KeyboardAvoidingScreen component:
- Uses proper default keyboard dismiss mode
- Configures appropriate vertical offset
- Maintains consistent styling
suite-native/module-send/src/screens/SendOutputsReviewScreen.tsx (2)
11-11: LGTM!The
Screenimport is correctly added to the existing navigation-related imports.
62-64: LGTM! Verify keyboard handling improvements.The component changes look good. The
Screencomponent is correctly used with theheaderprop replacingscreenHeader.Let's verify that the new
Screencomponent includes the keyboard handling improvements mentioned in the PR objectives:Also applies to: 74-74
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check if the Screen component uses react-native-keyboard-controller # Search for keyboard-related imports and usage in the Screen component ast-grep --pattern $'import { $_ } from "react-native-keyboard-controller"' # Check Screen component implementation ast-grep --pattern $'export const Screen = ({ $_ }) => { $$$ }'Length of output: 143
LGTM – Component structure and prop changes are solid. However, our automated search for direct keyboard handling references (e.g. use of "react-native-keyboard-controller" or explicit keyboard-related logic) in the
Screencomponent returned no matches. This result means we couldn’t automatically verify the keyboard handling improvements; please verify manually (or provide further context) to ensure that the newScreencomponent indeed incorporates the intended keyboard handling enhancements.
| @@ -80,6 +80,6 @@ export const SendDestinationTagReviewScreen = ({ | |||
| </Text> | |||
| <ReviewDestinationTagCard destinationTag={destinationTag} /> | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify keyboard handling in ReviewDestinationTagCard component.
Since this screen involves form input, ensure that the ReviewDestinationTagCard component properly utilizes the new keyboard handling capabilities.
Let's check the implementation:
🏁 Script executed:
#!/bin/bash
# Description: Check ReviewDestinationTagCard implementation for keyboard interaction
# Search for the component implementation
ast-grep --pattern $'export const ReviewDestinationTagCard = ({ $_ }) => {
$$$
}'
# Check for any text input components that might need keyboard handling
rg -A 5 "TextInput|Input" suite-native/module-send/src/components/ReviewDestinationTagCard.tsxLength of output: 1334
Keyboard Handling Not Found in ReviewDestinationTagCard Implementation
The component currently renders a styled card containing text (the destination tag) without any text inputs or interactive elements that typically require keyboard management. If the design anticipates keyboard interactions for form input (or editing), this component may need to be updated to include appropriate keyboard handling logic.
- The component does not render a TextInput or any focusable element.
- It lacks event handlers (e.g., onFocus, onBlur) related to keyboard interactions.
- Verify if the absence of keyboard handling is intentional; otherwise, incorporate the necessary modifications.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
suite-native/navigation/src/components/Screen.tsx (2)
154-154: Add TypeScript documentation for the new component.Consider adding JSDoc documentation to describe the component's purpose and keyboard handling behavior.
+/** + * A wrapper component that provides keyboard avoidance behavior using react-native-keyboard-controller. + * It wraps the base Screen component and adjusts its height when the keyboard is shown. + */ export const KeyboardAvoidingScreen = ({ children, ...rest }: ScreenProps) => {
150-152: Simplify the style definition.Since the style only contains
flex: 1, consider inlining it directly in the style prop.-const keyboardAvoidingScreenStyle = prepareNativeStyle(_ => ({ - flex: 1, -})); export const KeyboardAvoidingScreen = ({ children, ...rest }: ScreenProps) => { const { applyStyle, utils } = useNativeStyles(); return ( <KeyboardAvoidingView - style={applyStyle(keyboardAvoidingScreenStyle)} + style={{ flex: 1 }} keyboardVerticalOffset={utils.spacings.sp16} behavior="height" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
suite-native/navigation/src/components/Screen.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (3)
suite-native/navigation/src/components/Screen.tsx (3)
4-4: LGTM! Import aligns with PR objectives.The import of
KeyboardAvoidingViewfromreact-native-keyboard-controlleraligns with the PR's goal to improve keyboard handling in the native application.
28-28: Remove unused prop type.The
extraKeyboardAvoidingViewHeightprop is defined inScreenPropsbut is no longer used after migrating toreact-native-keyboard-controller.
154-166: LGTM! Well-structured keyboard handling implementation.The
KeyboardAvoidingScreencomponent is well-implemented with proper keyboard behavior configuration and good use of the composition pattern.
yanascz
force-pushed
the
fix/native/keyboard-aware-scroll-view
branch
8 times, most recently
from
29dcfd5 to
b41fa4f
Compare
suite-native/module-accounts-import/src/screens/XpubScanScreen.tsx
Outdated
Show resolved
Hide resolved
suite-native/navigation/src/components/ScreenContentWrapper.tsx
Outdated
Show resolved
Hide resolved
yanascz
force-pushed
the
fix/native/keyboard-aware-scroll-view
branch
from
a1963fc to
a49dc85
Compare
suite-native/app/package.json
Outdated
| @@ -114,7 +114,7 @@ | |||
| "react-native": "0.76.1", | |||
| "react-native-edge-to-edge": "^1.3.1", | |||
| "react-native-gesture-handler": "^2.21.0", | |||
| "react-native-keyboard-aware-scroll-view": "0.9.5", | |||
| "react-native-keyboard-controller": "patch:react-native-keyboard-controller@npm%3A1.16.5#~/.yarn/patches/react-native-keyboard-controller-npm-1.16.5-356a673ae8.patch", | |||
There was a problem hiding this comment.
This doesn't look right, other patched packages have only the version defined in package.json
yanascz
force-pushed
the
fix/native/keyboard-aware-scroll-view
branch
3 times, most recently
from
9c7f91e to
a139eab
Compare
| import Animated, { useAnimatedKeyboard, useAnimatedStyle } from 'react-native-reanimated'; | ||
|
|
||
| export const KeyboardFakeView = () => { | ||
| const keyboard = useAnimatedKeyboard(); |
There was a problem hiding this comment.
You should use similar hook from keyboard-controller because Reanimated is not handling correctly transluscent navbar on Android.
There was a problem hiding this comment.
This view is no longer necessary, found a solution using KeyboardStickyView. 😎
| @@ -0,0 +1,31 @@ | |||
| import * as React from 'react'; | |||
| import { EmitterSubscription, Keyboard, Platform } from 'react-native'; | |||
There was a problem hiding this comment.
I would suggest use of https://kirillzyusko.github.io/react-native-keyboard-controller/docs/api/keyboard-events instead of RN built-in to use one library for everything and be more consistent
There was a problem hiding this comment.
Fixed in 7430666, it's much shorter. 👍
yanascz
force-pushed
the
fix/native/keyboard-aware-scroll-view
branch
4 times, most recently
from
f96c26c to
ffa5c82
Compare
yanascz
force-pushed
the
fix/native/keyboard-aware-scroll-view
branch
from
ffa5c82 to
d5ab921
Compare
Resolves #16697