feat(suite-native) mobile FW revision check OfflineBanner #17014
Conversation
|
🚀 Expo preview is ready!
|
| @@ -12,13 +12,13 @@ | |||
| "dependencies": { | |||
| "@react-native-community/netinfo": "^11.4.1", | |||
| "@suite-native/atoms": "workspace:*", | |||
| "@suite-native/device": "workspace:*", | |||
There was a problem hiding this comment.
To extend the OfflineBanner for FW revision check, I need device here as a new dep, but:
- the 2nd commit on its own would cause circular deps
- because
deviceconsumesnavigation - and
navigationused to consume the useOffline thing fromconnection-status - now it gets the hook
useBannerAwareSafeAreaInsetsfromatoms - circular deps are avoided 🎉
| const bannerVariant = useAtomValue(deviceCompromisedBannerAtom); | ||
| const { applyStyle } = useNativeStyles(); | ||
| const { top: topSafeAreaInset } = useOfflineBannerAwareSafeAreaInsets(); |
There was a problem hiding this comment.
I added this component in #16959, but actually, this is wrong.
Yes, DeviceCompromisedBanner renders below OfflineBanner, so it needs to detect OfflineBanner to not use topSafeAreaInset.
But similarly, everything below the DeviceCompromisedBanner, should detect either of the two banners to not use topSafeAreaInset ❗
|
|
||
| const activeBannerMessages = useSelector(selectActiveBannerMessages); | ||
| const topInset = A.isNotEmpty(activeBannerMessages) ? topSafeAreaInset : 0; | ||
|
|
||
| if (A.isEmpty(activeBannerMessages)) return null; |
There was a problem hiding this comment.
this condition is newly needed, to prevent "empty padding" being rendered if none of the three banners in BannerRenderer is rendered.
Also it makes sense IMO, to omit the VStack container if there is nothing in it 🤷
| // If any banner is rendered, let it be known to components rendered below (e.g. Screen), | ||
| // so that they can adjust their top insets accordingly (via useBannerAwareSafeAreaInsets). | ||
| const isAnyBannerVisible = | ||
| isMessageSystemBannerVisible || isOfflineBannerVisible || isDeviceCompromisedBannerVisible; |
There was a problem hiding this comment.
This solves a tiny visual bug that existed even before I added the DeviceCompromisedBanner.
Aplying this comment analogically here:
any content rendered below MessageSystemBanner should detect if MessageSystemBanner is rendered, and not use topSafeAreaInset.
Before my PRs, if there is a msg system banner but not offline banner, the Screen has duplicate padding.
A tiny thing, but now it's done systematically, all at one place - easy to debug & extend in future 🙂
💭 Ofc, if there is a lot of these banners, a more complex solution will be needed. Otherwise, the size of this file will grow quadratically with the number of banners, but let's not overengineer it so soon 😄
trezor-ci
force-pushed
the
feat/mobile-FW-rev-check-banners-atom-renderer
branch
from
c5d37d0 to
10a2c27
Compare
Lemonexe
force-pushed
the
feat/mobile-FW-rev-check-OfflineBanner
branch
from
2845d35 to
2f2c406
Compare
Lemonexe
added
the
no-project
WalkthroughThe pull request refactors how banners and safe area insets are managed across the suite-native modules. A new dependency on ✨ 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 (4)
suite-native/atoms/src/useBannerAwareSafeAreaInsets.ts (1)
7-12: Consider enhancing the documentation.The JSDoc could be more comprehensive by including:
- Return type description
- Example usage
/** * Get top safe area inset, considering if there is any banner currently rendered. * If there is, then it already uses the safe inset (see BannersRenderer). * and we don't want to duplicate the inset for content below the banner. + * + * @returns {EdgeInsets} An object containing safe area insets with adjusted top value + * @example + * const insets = useBannerAwareSafeAreaInsets(); + * // { top: 0, bottom: 34, left: 0, right: 0 } // when banner is rendered + * // { top: 47, bottom: 34, left: 0, right: 0 } // when no banner is rendered */suite-native/app/src/BannersRenderer.tsx (1)
20-27: Consider memoizing the banner visibility check.The
isAnyBannerVisiblecalculation could be memoized usinguseMemoto prevent unnecessary recalculations, especially since it's used in both the effect and for conditional rendering.- const isAnyBannerVisible = - isMessageSystemBannerVisible || isOfflineBannerVisible || isDeviceCompromisedBannerVisible; + const isAnyBannerVisible = useMemo( + () => + isMessageSystemBannerVisible || + isOfflineBannerVisible || + isDeviceCompromisedBannerVisible, + [isMessageSystemBannerVisible, isOfflineBannerVisible, isDeviceCompromisedBannerVisible] + );suite-native/connection-status/src/OfflineBanner.tsx (1)
38-54: Consider adding accessibility attributes.The banner contains important status information. Consider enhancing accessibility by adding appropriate ARIA attributes.
- <VStack + <VStack + accessibilityRole="alert" + accessibilityLiveRegion="polite" spacing="sp2" alignItems="center" style={applyStyle(contentStyle, { topSafeAreaInset })} >suite-native/app/src/App.tsx (1)
11-13: LGTM! Improved banner management.Centralizing banner rendering into a single
BannersRenderercomponent improves maintainability and reduces component coupling.This architectural change:
- Simplifies banner management
- Reduces prop drilling
- Makes it easier to control banner ordering and visibility
Also applies to: 17-17, 67-67
📜 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 (28)
suite-native/app/package.json(1 hunks)suite-native/app/src/App.tsx(2 hunks)suite-native/app/src/BannersRenderer.tsx(1 hunks)suite-native/atoms/src/index.ts(1 hunks)suite-native/atoms/src/useBannerAwareSafeAreaInsets.ts(1 hunks)suite-native/connection-status/package.json(1 hunks)suite-native/connection-status/src/OfflineBanner.tsx(1 hunks)suite-native/connection-status/src/index.ts(0 hunks)suite-native/connection-status/src/useIsFwRevisionCheckOfflineError.ts(1 hunks)suite-native/connection-status/src/useIsOfflineBannerVisible.tsx(1 hunks)suite-native/connection-status/src/useOfflineBannerAwareSafeAreaInsets.tsx(0 hunks)suite-native/connection-status/tsconfig.json(1 hunks)suite-native/intl/src/en.ts(1 hunks)suite-native/message-system/package.json(0 hunks)suite-native/message-system/src/components/MessageSystemBannerRenderer.tsx(1 hunks)suite-native/message-system/tsconfig.json(0 hunks)suite-native/module-add-accounts/package.json(0 hunks)suite-native/module-add-accounts/src/screens/SelectAccountTypeScreen.tsx(2 hunks)suite-native/module-add-accounts/tsconfig.json(0 hunks)suite-native/module-authenticity-checks/package.json(0 hunks)suite-native/module-authenticity-checks/src/components/DeviceCompromisedBanner.tsx(2 hunks)suite-native/module-authenticity-checks/tsconfig.json(0 hunks)suite-native/navigation/package.json(0 hunks)suite-native/navigation/src/components/Screen.tsx(2 hunks)suite-native/navigation/tsconfig.json(0 hunks)suite-native/notifications/package.json(0 hunks)suite-native/notifications/src/components/NotificationRenderer.tsx(2 hunks)suite-native/notifications/tsconfig.json(0 hunks)
💤 Files with no reviewable changes (12)
- suite-native/module-authenticity-checks/package.json
- suite-native/message-system/tsconfig.json
- suite-native/navigation/package.json
- suite-native/message-system/package.json
- suite-native/module-add-accounts/package.json
- suite-native/module-authenticity-checks/tsconfig.json
- suite-native/connection-status/src/index.ts
- suite-native/notifications/package.json
- suite-native/module-add-accounts/tsconfig.json
- suite-native/notifications/tsconfig.json
- suite-native/connection-status/src/useOfflineBannerAwareSafeAreaInsets.tsx
- suite-native/navigation/tsconfig.json
⏰ 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 / 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: 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: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- 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: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (18)
suite-native/connection-status/tsconfig.json (1)
5-7: Verify the Added Device Module ReferenceThe addition of
{ "path": "../device" }in thereferencesarray is correctly integrated to support cross-module type-checking. Ensure that the device module exposes the required types and aligns with your monorepo structure.suite-native/connection-status/package.json (2)
15-15: Addition of@suite-native/deviceDependencyThe new dependency
"@suite-native/device": "workspace:*"is added as intended for extending the OfflineBanner to handle firmware revision check errors. This addition supports the decoupling of banner logic from navigation by leveraging the shared safe area inset hook from@suite-native/atoms, as referenced in the PR objectives and previous review comments.
13-14: Dependency Cleanup ConsistencyThe removal of
"react-native-safe-area-context"(as mentioned in the PR summary) appears to be part of the overall refactoring strategy to centralize safe area inset handling. Please ensure that no component or module in the codebase still depends on it.Also applies to: 16-23
suite-native/connection-status/src/useIsFwRevisionCheckOfflineError.ts (1)
1-9: LGTM! Well-structured hook with clear responsibility.The hook is well-implemented, focusing on a single responsibility of checking for the specific offline error condition.
suite-native/connection-status/src/useIsOfflineBannerVisible.tsx (1)
7-15: LGTM! Clean integration of the new offline error condition.The hook effectively combines the new firmware revision check offline error with the existing internet reachability check, maintaining a clear and logical flow.
suite-native/atoms/src/useBannerAwareSafeAreaInsets.ts (1)
5-18: LGTM! Well-implemented safe area insets management.The implementation effectively manages safe area insets while considering banner visibility, preventing duplicate insets.
suite-native/message-system/src/components/MessageSystemBannerRenderer.tsx (1)
17-38: LGTM! Clean implementation with proper type safety.The changes effectively:
- Extract banner visibility check into a reusable hook
- Add proper type safety with MessageSystemBannerRendererProps
- Handle empty state correctly
suite-native/notifications/src/components/NotificationRenderer.tsx (1)
4-4: LGTM!The changes correctly update the import and usage of the new
useBannerAwareSafeAreaInsetshook, maintaining the same functionality while aligning with the new banner management system.Also applies to: 21-21
suite-native/app/src/BannersRenderer.tsx (1)
17-48: LGTM! Well-structured implementation of centralized banner management.The component effectively manages multiple banners with proper safe area insets, ensuring only the first visible banner receives the top inset. The use of Jotai for state management is appropriate for this use case.
suite-native/connection-status/src/OfflineBanner.tsx (1)
24-26: LGTM! Clean type definition for props.The
OfflineBannerPropstype is well-defined and properly used in the component signature.suite-native/atoms/src/index.ts (1)
58-58: LGTM!The export follows the established pattern and correctly exposes the new hook.
suite-native/module-authenticity-checks/src/components/DeviceCompromisedBanner.tsx (3)
16-17: LGTM! Good visual separation.The added margin helps distinguish between banners with the same background color.
47-49: LGTM! Clean hook extraction.The hook provides a clean way to check banner visibility from other components.
50-52: LGTM! Proper prop typing.The component now explicitly accepts
topSafeAreaInsetprop with proper TypeScript typing.suite-native/navigation/src/components/Screen.tsx (1)
11-11: LGTM! Consistent inset handling.The change to use
useBannerAwareSafeAreaInsetsaligns with the centralized banner management approach.Also applies to: 107-107
suite-native/module-add-accounts/src/screens/SelectAccountTypeScreen.tsx (1)
15-15: LGTM! Consistent hook usage.The change to use
useBannerAwareSafeAreaInsetsmaintains consistency with the centralized banner management approach.Also applies to: 92-92
suite-native/intl/src/en.ts (1)
33-34: LGTM! Clear and informative offline message.The new translation key for firmware revision check errors provides clear information about both the issue and the solution.
suite-native/app/package.json (1)
108-108:❓ Verification inconclusive
Verify jotai version compatibility.
While pinning the version is good practice, version 1.9.1 seems outdated. Let's verify the latest version and any breaking changes.
🌐 Web query:
What is the latest version of jotai and are there any breaking changes between 1.9.1 and the latest version?💡 Result:
The latest version of Jotai is v2.12.0 as of February 2025[4][10]. Between v1.9.1 and v2.x versions, there are several significant breaking changes:
Key Breaking Changes
Async Atom Handling
Async atoms now behave like normal atoms with promise values rather than special-case handling:// Old (v1) - automatic promise resolution const derivedAtom = atom((get) => get(asyncAtom).toUpperCase()) // New (v2) - explicit async handling required const derivedAtom = atom(async (get) => (await get(asyncAtom)).toUpperCase())Utils like
splitAtomnow require sync atoms[1][3].Store API Changes
The Provider'sinitialValuesprop was removed in favor of direct store manipulation:import { createStore } from 'jotai/vanilla' const store = createStore() store.set(fooAtom, 'value')[1][7]TypeScript Changes
Writable atom type signature changed:// Old (v1) WritableAtom<Value, Arg, Result extends void | Promise<void>> // New (v2) WritableAtom<Value, Args extends unknown[], Result>[1]Utility Function Removals
Several utilities were deprecated/removed:
waitForAllreplaced by nativePromise.allabortableAtomfunctionality integrated into coreuseHydrateAtomsnow only works with writable atoms[1][4]Vanilla Separation
Core React functionality moved to separate entry points:// New import style import { atom } from 'jotai/vanilla' // Non-React core import { useAtom } from 'jotai' // React integration[1][7]Migration Recommendations
The official migration guide recommends using their codemod tools and testing with both versions using transitional APIs[1][3]. The Jotai team maintains detailed migration documentation at jotai.org/docs/guides/migrating-to-v2-api[1].
Citations:
- 1: https://jotai.org/docs/guides/migrating-to-v2-api
- 2: https://jquery.com/upgrade-guide/1.9/
- 3: RFC: New Jotai v2 API pmndrs/jotai#1514
- 4: https://github.com/pmndrs/jotai/releases
- 5: https://discourse.julialang.org/t/julia-1-8-1-9-rapid-release/85949
- 6: https://discourse.julialang.org/t/julia-1-9-1-testing-period/99522
- 7: https://jotai.org
- 8: https://jotai.org/docs/core/atom
- 9: https://app.studyraid.com/en/read/11290/352217/installing-jotai-in-a-react-project
- 10: https://www.npmjs.com/package/jotai?activeTab=versions
Jotai Version Upgrade Verification
The current pin in
suite-native/app/package.jsonis still on"jotai": "1.9.1", yet the latest version as of February 2025 is v2.12.0. Note that this major update comes with several breaking changes that affect:
- Async Atom Handling: Requires explicit async treatment rather than automatic resolution.
- Store API Changes: The
initialValuesprop on the Provider has been removed.- TypeScript Definitions: Writable atom type signatures have been modified.
- Utility Functions: Several previous utilities have been removed or replaced.
Recommendation:
Before upgrading, please review the official migration guide to assess the impact on your codebase—particularly if you’re leveraging async atoms, store manipulation, or specific utility functions. Ensure compatibility across your implementation if you choose to update to v2.12.0.
Description
👀 CR commit by commit recommended:
useOfflineBannerAwareSafeAreaInsetsinto something more generic.But why do I need that, isn't it overkill?
Related Issue
Followup to #16959
Resolve #16452
Screenshots:
Some combinations of banners. Not all of them though, it'd be unnecessary.
See that the topSafeAreaInset is correctly applied only to the first thing displayed..
none
Offline normal
Offline new
Offline new + MessageSystem
Offline new + DeviceCompromised
DeviceCompromised + MessageSystem
MessageSystem
multiple MessageSystem
[everything](https://github.com/user-attachments/assets/c6
EDIT: DeviceCompromised + MessageSystem critical (there's now a 1px white border)