fix(suite-native): pin matrix screen oveflow #17230
Conversation
WalkthroughThis pull request makes adjustments in two separate components. In the first component, a new state variable ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
suite-native/module-authorize-device/src/components/connect/PinOnKeypad.tsx (2)
89-89: Initialization default could cause layout shift.Setting isImageDisplayed to true by default might cause a flash of the image on initial render before layout measurements are taken, potentially causing a layout shift.
Consider using a null state initially and only showing the image after layout is measured:
-const [isImageDisplayed, setIsImageDisplayed] = useState(true); +const [isImageDisplayed, setIsImageDisplayed] = useState<boolean | null>(null);Then in the render:
-{isImageDisplayed && ( +{isImageDisplayed !== false && (
93-101: Document the threshold selection rationale.The current implementation uses 50% of screen height as the threshold, but there's no clear explanation for why this specific value was chosen.
Consider extracting this magic number to a named constant and documenting the rationale:
-const SCREEN_HEIGHT = getScreenHeight(); +// We hide the device image if the pin matrix takes up more than half the screen +// to prevent overflow issues on smaller devices +const SCREEN_HEIGHT = getScreenHeight(); +const PIN_MATRIX_HEIGHT_THRESHOLD = 0.5 * SCREEN_HEIGHT; // And then in the handler: -if (event.nativeEvent.layout.height > 0.5 * SCREEN_HEIGHT) { +if (event.nativeEvent.layout.height > PIN_MATRIX_HEIGHT_THRESHOLD) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
suite-native/module-authorize-device/src/components/connect/PinOnKeypad.tsx(3 hunks)suite-native/module-device-settings/src/components/DeviceInteractionScreenWrapper.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- suite-native/module-device-settings/src/components/DeviceInteractionScreenWrapper.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: EAS Update
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (4)
suite-native/module-authorize-device/src/components/connect/PinOnKeypad.tsx (4)
1-8: Appropriate imports added for new functionality.The added imports for useState, LayoutChangeEvent, and getScreenHeight provide the necessary tools to implement the responsive behavior needed to address the pin matrix overflow issue.
72-76: Good image styling approach for responsive display.The image styling with flex: 1, width: 100%, and maxHeight: 194 creates a responsive image that maintains proportions while setting appropriate constraints.
104-117: Clean responsive layout implementation.The responsive approach to conditionally rendering the image based on available space is well implemented. The flex justification and alignment properties ensure proper content placement when the image is hidden.
118-149: Effective event handling implementation.Using the onLayout event handler on the Box wrapper is an effective way to measure the pin matrix and make layout decisions. The approach correctly implements the PR objective to prevent overflow by conditionally displaying the image.
| maxHeight: 194, | ||
| })); | ||
|
|
||
| const SCREEN_HEIGHT = getScreenHeight(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider handling orientation changes for SCREEN_HEIGHT.
The SCREEN_HEIGHT constant is defined outside the component and won't update if orientation changes. If the app supports rotation, this could lead to incorrect display decisions.
🏁 Script executed:
#!/bin/bash
# Check if the app handles orientation changes elsewhere
rg -A 2 -B 2 "orientation|Orientation|portrait|landscape" --type tsLength of output: 20662
🏁 Script executed:
#!/bin/bash
# Search for usage of getScreenHeight in the repository to see if there's any dynamic updating
rg "getScreenHeight" --type tsLength of output: 2261
Action required: Dynamically update SCREEN_HEIGHT on orientation change
The current implementation of
const SCREEN_HEIGHT = getScreenHeight();defines SCREEN_HEIGHT at module load time, meaning its value won’t update when the device’s orientation changes. Given that orientation events are handled elsewhere in the app (e.g., via the orientationchange listener in SuiteLayout), you should consider one of the following approaches in this component:
- Move the call to
getScreenHeight()inside the component’s render or a hook (such as using state with an event listener on orientation change). - Subscribe to orientation or resize updates (using React Native’s
Dimensions.addEventListeneror equivalent) to update the screen height dynamically.
Please review and adjust the implementation to ensure that the component correctly reflects display changes after rotation.
|
🚀 Expo preview is ready!
|
| if (event.nativeEvent.layout.height > 0.5 * SCREEN_HEIGHT) { | ||
| setIsImageDisplayed(false); | ||
|
|
||
| return; | ||
| } | ||
| setIsImageDisplayed(true); |
There was a problem hiding this comment.
What about this?
| if (event.nativeEvent.layout.height > 0.5 * SCREEN_HEIGHT) { | |
| setIsImageDisplayed(false); | |
| return; | |
| } | |
| setIsImageDisplayed(true); | |
| setIsImageDisplayed(event.nativeEvent.layout.height <= 0.5 * SCREEN_HEIGHT); |
There was a problem hiding this comment.
good idea, fixed 11453ba
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
suite-native/module-authorize-device/src/components/connect/PinOnKeypad.tsx (1)
78-78:⚠️ Potential issueSCREEN_HEIGHT should be calculated inside the component
Defining
SCREEN_HEIGHToutside the component means it won't update if the device orientation changes. This could lead to incorrect display decisions after rotation.Move this constant inside the component and use a state variable with an effect to listen for dimension changes:
-const SCREEN_HEIGHT = getScreenHeight(); export const PinOnKeypad = ({ variant, onSuccess }: PinOnKeypadProps) => { + const [screenHeight, setScreenHeight] = useState(getScreenHeight()); + + useEffect(() => { + const dimensionsHandler = Dimensions.addEventListener( + 'change', + () => setScreenHeight(getScreenHeight()) + ); + + return () => dimensionsHandler.remove(); + }, []);Don't forget to import the necessary dependencies:
-import { useState } from 'react'; +import { useState, useEffect } from 'react'; +import { Dimensions } from 'react-native';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
suite-native/module-authorize-device/src/components/connect/PinOnKeypad.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (4)
suite-native/module-authorize-device/src/components/connect/PinOnKeypad.tsx (4)
72-76: Image style looks goodThe styles applied to the image will allow it to maintain an appropriate size while remaining responsive, which helps with the overflow issue.
89-89: State initialization looks goodUsing
useStateto track image visibility is a good approach for conditional rendering.
98-111: Improved VStack layout with conditional image renderingThe changes to the VStack, including
justifyContent="flex-end"and conditional rendering of the image, improve the layout's responsiveness.
112-143: Improved form layout structureWrapping the form in a Box with onLayout event handler is a good approach to handle responsive behavior based on available space.
| // Hide image if the screen is too small to fit both the image and the pin matrix. | ||
| const handlePinMatrixLayoutEvent = (event: LayoutChangeEvent) => | ||
| setIsImageDisplayed(event.nativeEvent.layout.height <= 0.5 * SCREEN_HEIGHT); | ||
|
|
There was a problem hiding this comment.
Logic seems inverted in handlePinMatrixLayoutEvent
The current condition hides the image when there should be plenty of space (when layout height is small). This seems counterintuitive.
- const handlePinMatrixLayoutEvent = (event: LayoutChangeEvent) =>
- setIsImageDisplayed(event.nativeEvent.layout.height <= 0.5 * SCREEN_HEIGHT);
+ const handlePinMatrixLayoutEvent = (event: LayoutChangeEvent) =>
+ setIsImageDisplayed(event.nativeEvent.layout.height < 0.5 * screenHeight);The exact condition might need further refinement based on your specific layout requirements. Consider this alternative that was suggested in a previous comment:
setIsImageDisplayed(event.nativeEvent.layout.height <= 0.5 * SCREEN_HEIGHT);
Committable suggestion skipped: line range outside the PR's diff.
Related Issue
Resolve #17035