Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(suite-native): return to Onboarding from DeviceCompromisedModal #16964

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Lemonexe
Copy link
Contributor

@Lemonexe Lemonexe commented Feb 12, 2025

Description

When closing DeviceCompromisedModalScreen, first let's try to navigate back, and only use the Home route as fallback.

There are three cases depending from where you enter the modal (
Actually, the FW revision check is done async by Connect (sometimes it may await http fetch), so basically the modal can be entered from anywhere. But it seems to me there are three typical cases from where you enter the modal:

  • HomeStack.Home: behaves the same
  • AuthorizeDeviceStackRoutes.ConnectingDevice: that worked, but with a glitch, because it'd first send you Home, but useHandleDeviceConnection would immediately redirect to ConnectingDevice, with disruptive animations. Now goes straight to ConnectingDevice (smoother)
  • OnboardingStack that was broken ❌ if modal was entered from onboarding, onboarding was effectively aborted (couldn't reenter it from Home), but now you are sent back to continue onboarding ✔️

Related Issue

Part of #16448

Screenshots

wipe app to reproduce:
Screencast from 2025-02-12 18-29-35.webm

@Lemonexe Lemonexe added mobile Suite Lite issues and PRs no-project This label is used to specify that PR doesn't need to be added to a project labels Feb 12, 2025
@Lemonexe Lemonexe marked this pull request as ready for review February 12, 2025 17:30
@Lemonexe Lemonexe requested a review from a team as a code owner February 12, 2025 17:30
Copy link

coderabbitai bot commented Feb 12, 2025

Walkthrough

The change updates the navigation logic in the modal screen component for device compromise. Previously, closing the modal always triggered a navigation to the Home screen. The updated implementation now checks if backward navigation is possible by using the navigation.canGoBack() method. If the condition is met, the component uses navigation.goBack() to navigate to the previous screen. If backward navigation is not available, it falls back to the original behavior by navigating to the Home screen. No modifications were made to the declarations of exported or public entities.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2663144 and 0bf7414.

📒 Files selected for processing (1)
  • suite-native/module-authenticity-checks/src/screens/DeviceCompromisedModalScreen.tsx (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: Setup and Cache Dependencies
  • GitHub Check: Analyze with CodeQL (javascript)
  • GitHub Check: prepare_android_test_app
🔇 Additional comments (2)
suite-native/module-authenticity-checks/src/screens/DeviceCompromisedModalScreen.tsx (2)

60-71: LGTM! Navigation logic improvement.

The updated navigation logic elegantly handles all three scenarios mentioned in the PR objectives:

  1. HomeStack.Home (via fallback)
  2. ConnectingDevice (via goBack)
  3. OnboardingStack (via goBack)

72-72: Verify the order of operations.

Consider if dismissCheck() should be called before navigation to prevent potential race conditions with effects that might fire after the check is dismissed.

Let's verify the impact:

❌ Verification inconclusive

🏁 Script executed:

#!/bin/bash
# Search for effects that might be triggered by dismissFirmwareAuthenticityCheck
rg -A 5 "useEffect.*dismissFirmwareAuthenticityCheck|dismissFirmwareAuthenticityCheck.*useEffect" 

Length of output: 99


I'll check the surrounding context in the DeviceCompromisedModalScreen.tsx file to understand the order of operations involving dismissCheck() and any subsequent navigation calls. Could you please run a shell script to search for dismissCheck and navigate calls in that file to verify their ordering?


🏁 Script executed:

#!/bin/bash
# Search for dismissCheck in the file to inspect the context
rg -n "dismissCheck" suite-native/module-authenticity-checks/src/screens/DeviceCompromisedModalScreen.tsx

# Search for navigation calls in the file to verify the order relative to dismissCheck
rg -n "navigate" suite-native/module-authenticity-checks/src/screens/DeviceCompromisedModalScreen.tsx

Length of output: 655


Based on our investigation, here’s what we found:

  • The file defines the dismissCheck function at line 52, and its invocation occurs at line 72.
  • The navigation call (navigating home) occurs at line 67, which is executed before the call to dismissCheck().
  • A code comment on line 58 explains that “after dismissCheck, an effect could fire in useHandleDeviceConnection to navigate away, but it's not guaranteed” — suggesting that even if the check is dismissed later, an alternate navigation might be triggered. However, the fallback navigation on line 67 guarantees the user isn’t left in an unintended state.
  • Our initial search for effects referencing the firmware authenticity check didn’t reveal any active hooks. This indicates that if such race conditions exist they aren’t manifesting through an easily identifiable effect or occur sporadically.

Given that manual navigation occurs before calling dismissCheck(), and no direct evidence of a harmful race condition was observed (nor any active effects that rely on the updated state from dismissCheck), the current ordering seems deliberate and functional. However, it might be worth a manual review (with additional runtime testing) to confirm that any potential side effects in useHandleDeviceConnection behave as expected.

Recommendation:
If runtime testing confirms that no unexpected side effects or race conditions occur, the current order should be acceptable. If any issues are observed, consider calling dismissCheck() before navigation.


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mobile Suite Lite issues and PRs no-project This label is used to specify that PR doesn't need to be added to a project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant