-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: ota update modal #24175
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
feat: ota update modal #24175
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
…mobile into wsun/ota-update-modal
…mobile into wsun/ota-update-modal
| return { | ||
| isCheckingUpdates, | ||
| }; | ||
| }, [navigation, otaUpdatesEnabled]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing cleanup for InteractionManager callback on unmount
The useEffect in useOTAUpdates schedules a navigation callback via InteractionManager.runAfterInteractions but doesn't return a cleanup function to cancel it. If the Main component unmounts (e.g., user logs out) while the callback is pending, the navigation to OTAUpdatesModal will still be attempted against a potentially stale navigation context. The runAfterInteractions returns a cancellable handle that could be stored and cancelled in a cleanup function returned from the effect.
…mobile into wsun/ota-update-modal
| return { | ||
| isCheckingUpdates, | ||
| }; | ||
| }, [navigation, otaUpdatesEnabled]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Effect may run multiple times despite documentation
The hook's documentation states it "Runs once when the app initially opens," but the useEffect includes navigation in its dependency array. The navigation object from useNavigation() may change identity between renders, causing the effect to re-run. Without a guard (like a useRef to track if the check already ran), this could trigger multiple checkForUpdateAsync calls and multiple navigations to the OTA modal when the navigation object changes.
| }); | ||
|
|
||
| it('checks for updates when feature flag is enabled', async () => { | ||
| mockSelectOtaUpdatesEnabledFlag.mockReturnValue(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests missing AAA blank line separation (Bugbot Rules)
The unit testing guidelines require every test to follow the AAA pattern with blank line separation between Arrange, Act, and Assert phases. In these tests, there's no blank line between the Arrange step (mockSelectOtaUpdatesEnabledFlag.mockReturnValue(false) or __DEV__ = true) and the Act step (renderHook(...)). This violates the rule: "EVERY test MUST follow the AAA pattern (Arrange, Act, Assert) with blank line separation."
Additional Locations (1)
| }); | ||
|
|
||
| it('checks for updates when feature flag is enabled', async () => { | ||
| mockSelectOtaUpdatesEnabledFlag.mockReturnValue(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests violate AAA pattern blank line separation rule (Bugbot Rules)
The first two tests in this file are missing blank line separation between Arrange and Act phases, violating the unit testing guidelines rule: "EVERY test MUST follow the AAA pattern (Arrange, Act, Assert) with blank line separation." In test does not check for updates when feature flag is disabled, line 84 (Arrange) directly precedes line 85 (Act) with no blank line. Same issue in skips update check in development mode even when feature flag is enabled between lines 93 and 94. The other tests in this file correctly follow the pattern with blank lines between all phases.
Additional Locations (1)
…mobile into wsun/ota-update-modal
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsThis PR introduces a new OTA (Over-The-Air) Updates Modal feature with the following key changes:
Risk Assessment:
Tag Selection Rationale:
The changes are self-contained to the OTA updates feature and don't touch other functional areas like accounts, swaps, confirmations, etc. |
|
Cal-L
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM



Description
Implement OTA Update modal. On iOS, if there are updates available, users can reload the app to see the updates. On Android, the app crashes when we call reloadAsync so we let users know that they need to close and reopen the app to receive the updates
Changelog
CHANGELOG entry: Added OTA updates modal
Related issues
Fixes: #24110
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Adds an OTA-driven update UX using a modal and removes render-blocking during update checks.
OTAUpdatesModalbottom sheet with platform-specific copy and primary action (iOS reload viareloadAsync); tracked withMetaMetricsEventsand covered by testsuseOTAUpdatesto: respect feature flag, skip in__DEV__, check/fetch updates, and navigate toOTAUpdatesModalafter interactions whenisNewis true; no longer reloads automatically or gate-renders viaFoxLoaderAppand registers routeRoutes.MODAL.OTA_UPDATES_MODAL; updatesApp.test.tsxto remove loader assertions and mockuseOTAUpdatesOTA_UPDATES_MODAL_VIEWED,OTA_UPDATES_MODAL_PRIMARY_ACTION_CLICKED) and English i18n strings for modal contentWritten by Cursor Bugbot for commit bedde63. This will update automatically on new commits. Configure here.