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

feat(mobile): update FW alert on Home + remote feature flag #16318

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Nodonisko
Copy link
Contributor

@Nodonisko Nodonisko commented Jan 12, 2025

Description

  1. Added update alert box to Homescreen
  2. Added remote feature flag for message system to disable => tested with local JWS config + also retested local feature flag just to be sure

Related Issue

Resolve part of #15584

Screenshots:

screen-20250113-145307.mp4

Copy link

github-actions bot commented Jan 12, 2025

🚀 Expo preview is ready!

  • Project → trezor-suite-preview
  • Platforms → android, ios
  • Scheme → trezorsuitelite
  • Runtime Version → 21
  • More info

Learn more about 𝝠 Expo Github Action

@Nodonisko Nodonisko force-pushed the feat/update-firmware-alert branch from cb30b94 to 215e8f9 Compare January 13, 2025 14:22
@Nodonisko Nodonisko marked this pull request as ready for review January 13, 2025 14:33
@Nodonisko Nodonisko requested review from a team, tomasklim and matejkriz as code owners January 13, 2025 14:33
@Nodonisko Nodonisko force-pushed the feat/update-firmware-alert branch from 215e8f9 to b512d83 Compare January 13, 2025 14:33
@Nodonisko Nodonisko changed the title feat(mobile): update FW alert on homescreen feat(mobile): update FW alert on Home + remote feature flag Jan 13, 2025
const AnimatedStack = Animated.createAnimatedComponent(Stack);
AnimatedStack.displayName = 'AnimatedStack';
export const AnimatedVStack = AnimatedStack;
export const AnimatedHStack = AnimatedStack;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is orientation missing here intentionally? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed

@Nodonisko Nodonisko force-pushed the feat/update-firmware-alert branch from b512d83 to 5305247 Compare January 14, 2025 13:33
@Nodonisko Nodonisko force-pushed the feat/update-firmware-alert branch from 5305247 to 6472290 Compare January 14, 2025 13:34
@matejkriz matejkriz added the mobile Suite Lite issues and PRs label Jan 14, 2025
@@ -25,6 +25,7 @@ export const Feature = {
ethClaim: 'eth.staking.claim',
firmwareRevisionCheck: 'security.firmware.check',
firmwareHashCheck: 'security.firmware.hashCheck',
firmwareUpdate: 'device.firmware.update',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking whether we should indicate somehow, that this feature flag is implemented on mobile only. My initial thought was using mobile prefix in the id, but since those features can be implemented later on the other platform, I'm not proposing that. We can use comments if needed. Feel free to keep it as it is.

selectIsFeatureEnabled(state, Feature.firmwareUpdate, true),
);

const isFirmwareUpdateEnabled = localFeatureFlag && remoteFeatureFlag;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way it wouldn't be available on production for users at all, because localFeatureFlag has default value isDevelopOrDebugEnv(). And there is no way to enable it locally if it's remotely disabled. I guess OR makes more sense here.

Suggested change
const isFirmwareUpdateEnabled = localFeatureFlag && remoteFeatureFlag;
const isFirmwareUpdateEnabled = localFeatureFlag || remoteFeatureFlag;

Copy link
Member

@matejkriz matejkriz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except the wrong condition in useIsFirmwareUpdateFeatureEnabled.tsx
it works and looks good, including possibility to disable via message-system. It's nice that it's animated.

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
Projects
Status: 🏃‍♀️ In progress
Development

Successfully merging this pull request may close these issues.

3 participants