Add Desktop UI #78

OGPoyraz · 2022-10-12T07:36:23Z

Important Note: This PR is going to get merged into include-desktop-ui-builder PR (already approved), then I will merge include-desktop-ui-builder into main-desktop

How to run it locally?

No changes in the mmd start scripts. Follow the steps on app/scripts/desktop/Readme.md

Important parts of the MMD-UI

Locales

Most of the code(i18nProvider, getMessage, translate function etc.) was copied from MM with few changes. In the long run, it would be more maintainable for us to have separated locales; hence I've put the MMD locales in a separate folder.

State Management

I wanted to keep the same tooling as the MM, so I kept Redux.
I believe we will need some data persistence in the MMD (E.g desktop app settings). For that purpose I've used redux-persist with electron-store

Styling

To use MM components, we are injecting the monolith CSS of MM. (See app/desktop-ui.html)
It's not ideal, but good to go for the beginning. Since we import MM css file, I've added mmd.scss to the MM index scss so we create and use our styles under desktop-ui.

Communication between Renderer and Main process

It's worth mentioning; we are registering listeners in the desktop-ui/index.js (E.g registerUpdatePairStatus) if we need that state in the higher scope.
And for the other way; we invoke a message from the renderer (E.g desktop-ui/pages/pair/pair.component.js see handleOTPChange)

Known issues

window.require('electron')

There are weird bundling issues if we try to import things from electron package.
To reproduce: Find & replace const { ipcRenderer } = window.require('electron'); to import { ipcRenderer } from 'electron'

Flaky active connection state

The state communication between MMD and MM is getting improved by Vinicius PR, so be aware if you see some discrepancies through your local tests
I've added Temporary Pair Page Link in the deactivated state so you can easily navigate the pair page if needed.

No message is sent back to MMD if the user disconnects from MM

We will have a look at this issue soon.

OTP component

Not an issue but worth mentioning. For the sake of moving fast, I've used a very basic(0 dependency) package to handle OTP input. We can create our own if we need it in the future.

Todos

Error pages
Unit tests

app/desktop-ui.html

development/build/scripts.js

development/build/static.js

vinistevam · 2022-10-12T12:31:06Z

app/desktop-ui.html

+  <head>
+    <meta charset="utf-8">
+    <meta name="viewport" content="width=device-width, initial-scale=1 user-scalable=no">
+    <title>MetaMask</title>


Just add "MetaMask Desktop" here, but probably It will be replaced by the UI Pairing flow ticket.

vinistevam

It's really good to see it's working using MM build scripts 🥇 .
Just run lint:fix and 👍

desktop-ui/App.js

package.json

development/build/index.js

Co-authored-by: Matthew Walsh <matthew.walsh@consensys.net>

matthewwalsh0 · 2022-10-26T14:07:34Z

app/scripts/desktop.js

@@ -0,0 +1,9 @@
+import launchDesktopUi from '../../ui/desktop';


To make it explicit that this is running in a renderer process and not part of the background, shall we call this something like ui.js and move it inside the desktop directory?

This makes sense, I've tried to solve it quickly, but if we move that file into a different folder, we need to make many modifications to build scripts.

For example, changing the desktop entry point in standartEntryPoints(development/build/scripts.js#232) to desktop/ui causes different bundler requirements (bundler getting confused about folders, e.g lavamoat options development/build/scripts.js#557).

It's not the best but rather than moving the file, I've renamed it to desktop-ui.js. Shall we create a task for that and have a look later on?

indeed, changing the structure will complicate the build scripts. I'm in favour to create a task in the refactor section to tackle it later.

matthewwalsh0 · 2022-10-26T14:09:18Z

app/scripts/desktop/app/desktop-app.ts

@@ -175,27 +175,31 @@ class DesktopApp extends EventEmitter {

  private async createStatusWindow() {
    const statusWindow = new BrowserWindow({
-      width: 800,
-      height: 400,
+      width: process.env.METAMASK_DEBUG ? 1800 : 1000,


This seems a little big to load with as it hits the bounds of my 16" laptop at the default zoom level. Maybe start small but let the user maximise or persist after resize?

matthewwalsh0 · 2022-10-26T14:10:28Z

app/scripts/desktop/app/desktop-app.ts

@@ -175,27 +175,31 @@ class DesktopApp extends EventEmitter {

  private async createStatusWindow() {


Shall we change all these references for clarity and maybe refer to this as the main window?

matthewwalsh0 · 2022-10-26T16:02:35Z

app/scripts/desktop/app/desktop-app.ts

      webPreferences: {
-        preload: path.resolve(__dirname, './status-preload.js'),


Whether we ultimately use a preload script or not, we can delete this file and any old logic if not used.

I've added back this line to use version and other use cases from status-preload.ts

socket-security · 2022-10-27T11:41:52Z

Socket Security Report

👍 No new dependency issues detected in pull request

Socket.dev scan summary

Issue	Status
Did you mean?	✅ no new possible package typos
Install scripts	✅ no new install scripts
Telemetry	✅ no new telemetry
Troll package	✅ no new troll packages
Malware	✅ no new malware
Native code	✅ no new native modules
Bin Script Confusion	✅ no new bin script confusions
Bin script shell injection	✅ no new bin script shell injection

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@2.4.2

Powered by socket.dev

cryptotavares · 2022-10-27T12:18:28Z

development/build/static.js

@@ -46,7 +47,7 @@ module.exports = function createStaticAssetTasks({
    composeSeries(
      ...copyTargetsProd.map((target) => {
        return async function copyStaticAssets() {
-          await performCopy(target);
+          await performCopy(target, includeDesktopUi);


There's no need to add includeDesktopUi.. it is already available in the higher scope. Also that method signature only has 1 arg 😅

Thanks, it's been forgotten one :D

cryptotavares · 2022-10-27T12:21:47Z

package.json

@@ -13,12 +13,13 @@
    "start": "yarn build:dev dev --apply-lavamoat=false",
    "build:desktop": "./development/build-desktop.sh",
    "build:desktop:mv3": "ENABLE_MV3=true ./development/build-desktop.sh",
-    "build:desktop:extension": "DESKTOP=EXTENSION yarn start --build-type flask",
+    "build:desktop:extension": "DESKTOP=EXTENSION yarn start --build-type flask --include-desktop-ui true",


we should split this.. Currently this will generate the bundle for the extension and the desktop app ui. Those are 2 different things and should be dealt separately (even if the bundle process is the same).

IMO it would make more sense if the build:desktop would actually generate both the main and renderer process "bundles" (as both are part of the desktop app)

OGPoyraz requested a review from a team as a code owner October 12, 2022 07:36

OGPoyraz commented Oct 12, 2022

View reviewed changes

app/desktop-ui.html Show resolved Hide resolved

OGPoyraz commented Oct 12, 2022

View reviewed changes

development/build/scripts.js Show resolved Hide resolved

OGPoyraz commented Oct 12, 2022

View reviewed changes

development/build/static.js Show resolved Hide resolved

vinistevam reviewed Oct 12, 2022

View reviewed changes

vinistevam approved these changes Oct 12, 2022

View reviewed changes

OGPoyraz force-pushed the include-desktop-ui-builder branch 2 times, most recently from d56be00 to fef377a Compare October 13, 2022 07:53

OGPoyraz force-pushed the include-desktop-ui-builder branch 2 times, most recently from 7b47b56 to 1ff1514 Compare October 20, 2022 07:31

matthewwalsh0 reviewed Oct 24, 2022

View reviewed changes

desktop-ui/App.js Outdated Show resolved Hide resolved

matthewwalsh0 reviewed Oct 24, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

matthewwalsh0 reviewed Oct 24, 2022

View reviewed changes

development/build/index.js Outdated Show resolved Hide resolved

OGPoyraz changed the title ~~Include Desktop UI builder scripts~~ Include Desktop UI Oct 25, 2022

OGPoyraz force-pushed the include-desktop-ui-builder branch from 9cf3244 to 6d6daff Compare October 25, 2022 07:24

OGPoyraz changed the title ~~Include Desktop UI~~ Add Desktop UI Oct 25, 2022

OGPoyraz requested a review from vinistevam October 25, 2022 07:25

OGPoyraz force-pushed the include-desktop-ui-builder branch 4 times, most recently from ceb1bf7 to 7ac9ce0 Compare October 26, 2022 10:24

OGPoyraz and others added 5 commits October 26, 2022 12:30

Include Desktop UI builder flow

ab5c233

Add dist desktop folder to eslint ignore patterns

8fd04ac

Fix lint issues, add desktop-ui react app into eslintrc.js

d31ea70

Add desktop React app (#86)

215c992

Update development/build/index.js

ea4f216

Co-authored-by: Matthew Walsh <matthew.walsh@consensys.net>

OGPoyraz force-pushed the include-desktop-ui-builder branch from 7ac9ce0 to ea4f216 Compare October 26, 2022 10:41

matthewwalsh0 reviewed Oct 26, 2022

View reviewed changes

OGPoyraz added 7 commits October 27, 2022 09:04

Change desktop-ui entry script name

76702dd

Change initial window size of desktop-ui

a1700c6

Rename statusWindow to mainWindow

be25eff

Add DESKTOP_UI_DEBUG env var

9441ba8

Change testMatch to higher scope

c8c1875

Rename dist script name for desktop-ui

aa305fb

Add choose preffered language paragraph

f01041e

OGPoyraz force-pushed the include-desktop-ui-builder branch from 2cd93e9 to f01041e Compare October 27, 2022 09:22

OGPoyraz added 3 commits October 27, 2022 11:27

Modify currentLanguage copies

0000369

Add back status-preload

2ba7c41

Add MMD electron icon path

d7aef84

Depend isWebSocketConnected property for connection status

5cd81e8

OGPoyraz force-pushed the include-desktop-ui-builder branch from 1b5aee0 to 5cd81e8 Compare October 27, 2022 11:53

cryptotavares reviewed Oct 27, 2022

View reviewed changes

Remove includeDesktopUi from performCopy

f841b2d

cryptotavares approved these changes Oct 27, 2022

View reviewed changes

Fix tslint

a7c342b

vinistevam approved these changes Oct 27, 2022

View reviewed changes

Decrease global test coverage

9eebc14

OGPoyraz force-pushed the include-desktop-ui-builder branch from b1bf0cb to 9eebc14 Compare October 28, 2022 06:52

Merge branch 'main-desktop' into include-desktop-ui-builder

904e9c5

matthewwalsh0 approved these changes Oct 28, 2022

View reviewed changes

OGPoyraz merged commit e1952d1 into main-desktop Oct 28, 2022

This was referenced Nov 1, 2022

Make use of "ContextBridge" for modules needed in desktop-ui #171

Closed

Separate desktop-ui building scripts from extension build process #172

Closed

Investigate how we can load React and storybook's components into desktop app ui #183

Closed

gauthierpetetin deleted the include-desktop-ui-builder branch November 4, 2022 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Desktop UI #78

Add Desktop UI #78

OGPoyraz commented Oct 12, 2022 •

edited

vinistevam Oct 12, 2022

vinistevam left a comment

matthewwalsh0 Oct 26, 2022

OGPoyraz Oct 27, 2022

vinistevam Oct 27, 2022

matthewwalsh0 Oct 26, 2022

OGPoyraz Oct 27, 2022

matthewwalsh0 Oct 26, 2022

OGPoyraz Oct 27, 2022

matthewwalsh0 Oct 26, 2022

OGPoyraz Oct 27, 2022

OGPoyraz Oct 27, 2022

socket-security bot commented Oct 27, 2022 •

edited

cryptotavares Oct 27, 2022

OGPoyraz Oct 27, 2022

OGPoyraz Oct 27, 2022

cryptotavares Oct 27, 2022

		@@ -0,0 +1,9 @@
		import launchDesktopUi from '../../ui/desktop';

		@@ -175,27 +175,31 @@ class DesktopApp extends EventEmitter {

		private async createStatusWindow() {

		webPreferences: {
		preload: path.resolve(__dirname, './status-preload.js'),

Add Desktop UI #78

Add Desktop UI #78

Conversation

OGPoyraz commented Oct 12, 2022 • edited

How to run it locally?

Important parts of the MMD-UI

Locales

State Management

Styling

Communication between Renderer and Main process

Known issues

window.require('electron')

Flaky active connection state

No message is sent back to MMD if the user disconnects from MM

OTP component

Todos

Choose a reason for hiding this comment

vinistevam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

socket-security bot commented Oct 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGPoyraz commented Oct 12, 2022 •

edited

socket-security bot commented Oct 27, 2022 •

edited