chore(suite-native): refactor navigation route matching #16965
Conversation
Lemonexe
added
mobile
| // If the device is connected again, he still should stay on that screen. | ||
| const isSuspiciousDeviceScreenFocused = useNavigationRouteMatch( | ||
| OnboardingStackRoutes.SuspiciousDevice, | ||
| ); |
There was a problem hiding this comment.
In this case we want to match the most specific route.
For that purpose we can use useNavigationRouteMatch, introduced in #16959.
ofc useRoute would be best, but that's not possible here
| OnboardingStackRoutes.SuspiciousDevice, | ||
| ); | ||
|
|
||
| const lastRoute = useNavigationState(state => state?.routes.at(-1)?.name); |
There was a problem hiding this comment.
Actually, the current implementation may be buggy! TL;DR useNavigateState ensures that we'll rerender when the state changes (or more precisely - when the selector return value changes, similarly to redux useSelector)
💭 these cases are not matching the most specific route, but rather the first stack under RootStackNavigator. useNavigationRouteMatch only matches the lowest (most specific) route. I guess I could refactor it to gradually match at any hierarchy level, but for such simple case I think that useNavigationState directly is fine 🙂
Lemonexe
force-pushed
the
feat/mobile-FW-rev-check-banners-atom-renderer
branch
from
66ae21b to
d0b9416
Compare
Lemonexe
force-pushed
the
chore/refactor-navigation-route-matching
branch
from
2751acd to
88c32b7
Compare
|
🚀 Expo preview is ready!
|
trezor-ci
force-pushed
the
feat/mobile-FW-rev-check-banners-atom-renderer
branch
from
c5d37d0 to
10a2c27
Compare
Lemonexe
force-pushed
the
chore/refactor-navigation-route-matching
branch
from
88c32b7 to
c1e5c37
Compare
WalkthroughThe changes update the navigation state management in the device connection hook. The previous implementation used a separate function to determine if the current screen was the suspicious device screen. This approach has been replaced by directly using ✨ 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: 0
🧹 Nitpick comments (1)
suite-native/device/src/hooks/useHandleDeviceConnection.ts (1)
178-180: Consider implementing the suggested TODO improvement.The TODO comment suggests a potential improvement to simplify the complex navigation logic by introducing a single source of truth. This would make the code more maintainable and easier to understand.
Consider refactoring to use a single function that determines the target route based on the current state, replacing the multiple useEffect hooks with imperative navigation instructions. This would make the navigation logic more declarative and easier to maintain.
Example approach:
const getTargetRoute = (state: AppState): Route => { if (state.isFirmwareInstallationRunning) return currentRoute; if (state.isDeviceCompromisedModalFocused) return currentRoute; if (state.isSuspiciousDeviceScreenFocused) return currentRoute; // ... other conditions return { name: RootStackRoutes.AppTabs, params: { ... } }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
suite-native/device/src/hooks/useHandleDeviceConnection.ts(9 hunks)
⏰ 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 (3)
suite-native/device/src/hooks/useHandleDeviceConnection.ts (3)
4-4: LGTM! Import changes align with refactoring goals.The addition of navigation hooks from '@react-navigation/native' and '@suite-native/navigation' supports the route matching refactoring.
Also applies to: 30-30
72-80: LGTM! Navigation state management improvements.The changes improve the route matching logic in two ways:
useNavigationRouteMatchensures matching the most specific route for the suspicious device screen.useNavigationStateguarantees proper re-rendering when navigation state changes.The added comments clearly explain the suspicious device screen behavior.
88-88: LGTM! Consistent effect hook changes.The suspicious device screen check is consistently added to all relevant useEffect hooks and their dependency arrays, ensuring proper handling of the suspicious device screen state.
Also applies to: 114-114, 124-124, 165-165, 181-181, 198-198
Description
Tweak navigation route matching in
useHandleDeviceConnection. I searched for all occurrences of.routes, but did not find any other files which could do with such refactoring.I tested that in following cases, the navigation behavior is unchanged:
Related Issue
Related to #16452