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.

Sign up for GitHub

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

Jump to bottom

AB tests followup #16243

Merged
merged 3 commits into from
Jan 14, 2025
Merged

AB tests followup #16243

merged 3 commits into from
Jan 14, 2025

Conversation

matejkriz
Copy link
Member

@matejkriz matejkriz commented Jan 7, 2025 •
edited
Loading

Description

  • refactor ab testing to follow code style and make it easier to reuse on mobile
  • load valid experiments to store also for mobile app

Related Issue

Followup #15067

Screenshots

Redux state:
image

Example use of useExperiment on mobile:
image

Result:
image

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 7, 2025 •
edited
Loading

🚀 Expo preview is ready!

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

Learn more about 𝝠 Expo Github Action

@matejkriz matejkriz mentioned this pull request Jan 7, 2025
Merged
1 task
@matejkriz matejkriz marked this pull request as ready for review January 7, 2025 15:37
@matejkriz matejkriz requested review from a team and tomasklim as code owners January 7, 2025 15:37
@matejkriz matejkriz added mobile Suite Lite issues and PRs message-system Updates in secure message system config labels Jan 7, 2025
@matejkriz
Copy link
Member Author

I was thinking about merging messageSystemMiddlewares for desktop and native, since they are almost the same, but they are not exactly same and there is some import from suite package on desktop, so I believe it's better to tolerate the duplication here and keep them separated, but not a strong opinion.

@PeKne PeKne assigned PeKne and matejkriz and unassigned PeKne Jan 8, 2025
adderpositive
adderpositive approved these changes Jan 8, 2025
View reviewed changes
Copy link
Contributor

@adderpositive adderpositive left a comment

Choose a reason for hiding this comment

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

Thank you for the excellent fine-tuning and extended functionality on mobile. I had missed messageSystemMiddleware, so thank you for including it.

@matejkriz
Copy link
Member Author

/rebase

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 9, 2025

Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/12693630510

@trezor-ci trezor-ci force-pushed the feat/ab-testing-followup branch from d51287a to bced006 Compare January 9, 2025 15:58
@matejkriz matejkriz force-pushed the feat/ab-testing-followup branch from bced006 to bb29e8c Compare January 9, 2025 16:23
@matejkriz
Copy link
Member Author

/rebase

@github-actions GitHub Actions
Copy link

Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/12707203513

@trezor-ci trezor-ci force-pushed the feat/ab-testing-followup branch from bb29e8c to e987a97 Compare January 10, 2025 09:53
yanascz
yanascz reviewed Jan 10, 2025
View reviewed changes
suite-common/message-system/src/messageSystemUtils.ts Outdated
config: MessageSystem | null,
options: Options,
): ExperimentsItem[] => {
export const getValidExperiments = (config: MessageSystem | null, options: Options): string[] => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand correctly that this functions returns experiment IDs? 🤔

If yes, then I'd suggest different naming:

Suggested change
export const getValidExperiments = (config: MessageSystem | null, options: Options): string[] => {
export const getValidExperimentIds = (config: MessageSystem | null, options: Options): string[] => {

It'd also be great to make the return type more strict if possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

4568889 Renamed. The config type is auto-generated so I would not use branded type here. It can be used, but I see much more important candidates for more strict types everywhere around.

yanascz
yanascz approved these changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@yanascz yanascz left a comment

Choose a reason for hiding this comment

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

Nice and clean!

@matejkriz matejkriz force-pushed the feat/ab-testing-followup branch from 4568889 to 364a9db Compare January 14, 2025 09:43
@matejkriz matejkriz merged commit 97ea53c into develop Jan 14, 2025
79 checks passed
@matejkriz matejkriz deleted the feat/ab-testing-followup branch January 14, 2025 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yanascz yanascz yanascz approved these changes

@adderpositive adderpositive adderpositive approved these changes

@tomasklim tomasklim Awaiting requested review from tomasklim tomasklim is a code owner

Assignees

@matejkriz matejkriz

Labels
message-system Updates in secure message system config mobile Suite Lite issues and PRs
Projects
Suite Mobile
Status: 🤝 Needs QA
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@matejkriz @adderpositive @yanascz @PeKne